diff --git a/pkg/services/ngalert/accesscontrol/models.go b/pkg/services/ngalert/accesscontrol/models.go index 1f0403a18f7..ba8b0e734e2 100644 --- a/pkg/services/ngalert/accesscontrol/models.go +++ b/pkg/services/ngalert/accesscontrol/models.go @@ -1,9 +1,28 @@ package accesscontrol import ( - "errors" + "fmt" + + "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/util/errutil" ) var ( - ErrAuthorization = errors.New("user is not authorized") + errAuthorizationGeneric = errutil.Unauthorized("alerting.unauthorized") ) + +func NewAuthorizationErrorWithPermissions(action string, eval accesscontrol.Evaluator) error { + msg := fmt.Sprintf("user is not authorized to %s", action) + err := errAuthorizationGeneric.Errorf(msg) + err.PublicMessage = msg + if eval != nil { + err.PublicPayload = map[string]any{ + "permissions": eval.GoString(), + } + } + return err +} + +func NewAuthorizationErrorGeneric(action string) error { + return NewAuthorizationErrorWithPermissions(action, nil) +} diff --git a/pkg/services/ngalert/accesscontrol/rules.go b/pkg/services/ngalert/accesscontrol/rules.go index e6c40df8d98..bc1fd23b5be 100644 --- a/pkg/services/ngalert/accesscontrol/rules.go +++ b/pkg/services/ngalert/accesscontrol/rules.go @@ -6,7 +6,6 @@ import ( "golang.org/x/net/context" "github.com/grafana/grafana/pkg/expr" - "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/dashboards" @@ -22,8 +21,6 @@ const ( ruleDelete = accesscontrol.ActionAlertingRuleDelete ) -var logger = log.New("ngalert.accesscontrol") - type RuleService struct { ac accesscontrol.AccessControl } @@ -34,51 +31,92 @@ func NewRuleService(ac accesscontrol.AccessControl) *RuleService { } } -// HasAccess returns true if the user has all permissions specified by the evaluator -func (r *RuleService) HasAccess(ctx context.Context, user identity.Requester, evaluator accesscontrol.Evaluator) bool { - result, err := r.ac.Evaluate(ctx, user, evaluator) - if err != nil { // this is how accesscontrol.HasAccess works. //TODO change when AuthorizeDatasourceAccessForRule can return errors - logger.FromContext(ctx).Error("Failed to evaluate access control", "error", err) - return false +// HasAccess returns true if the identity.Requester has all permissions specified by the evaluator. Returns error if access control backend could not evaluate permissions +func (r *RuleService) HasAccess(ctx context.Context, user identity.Requester, evaluator accesscontrol.Evaluator) (bool, error) { + return r.ac.Evaluate(ctx, user, evaluator) +} + +// HasAccessOrError returns nil if the identity.Requester has enough permissions to pass the accesscontrol.Evaluator. Otherwise, returns authorization error that contains action that was performed +func (r *RuleService) HasAccessOrError(ctx context.Context, user identity.Requester, evaluator accesscontrol.Evaluator, action func() string) error { + has, err := r.HasAccess(ctx, user, evaluator) + if err != nil { + return err } - return result + if !has { + return NewAuthorizationErrorWithPermissions(action(), evaluator) + } + return nil +} + +// getRulesReadEvaluator constructs accesscontrol.Evaluator that checks all permission required to read all provided rules +func (r *RuleService) getRulesReadEvaluator(rules ...*models.AlertRule) accesscontrol.Evaluator { + return r.getRulesQueryEvaluator(rules...) +} + +// getRulesQueryEvaluator constructs accesscontrol.Evaluator that checks all permissions to query data sources used by the provided rules +func (r *RuleService) getRulesQueryEvaluator(rules ...*models.AlertRule) accesscontrol.Evaluator { + added := make(map[string]struct{}, 2) + evals := make([]accesscontrol.Evaluator, 0, 2) + for _, rule := range rules { + for _, query := range rule.Data { + if query.QueryType == expr.DatasourceType || query.DatasourceUID == expr.DatasourceUID || query. + DatasourceUID == expr.OldDatasourceUID { + continue + } + if _, ok := added[query.DatasourceUID]; ok { + continue + } + evals = append(evals, accesscontrol.EvalPermission(datasources.ActionQuery, datasources.ScopeProvider.GetResourceScopeUID(query.DatasourceUID))) + added[query.DatasourceUID] = struct{}{} + } + } + if len(evals) == 1 { + return evals[0] + } + return accesscontrol.EvalAll(evals...) } // AuthorizeDatasourceAccessForRule checks that user has access to all data sources declared by the rule -func (r *RuleService) AuthorizeDatasourceAccessForRule(ctx context.Context, user identity.Requester, rule *models.AlertRule) bool { - for _, query := range rule.Data { - if query.QueryType == expr.DatasourceType || query.DatasourceUID == expr.DatasourceUID || query. - DatasourceUID == expr.OldDatasourceUID { - continue +func (r *RuleService) AuthorizeDatasourceAccessForRule(ctx context.Context, user identity.Requester, rule *models.AlertRule) error { + ds := r.getRulesQueryEvaluator(rule) + return r.HasAccessOrError(ctx, user, ds, func() string { + suffix := "" + if rule.UID != "" { + suffix = fmt.Sprintf(" of the rule UID '%s'", rule.UID) } - if !r.HasAccess(ctx, user, accesscontrol.EvalPermission(datasources.ActionQuery, datasources.ScopeProvider.GetResourceScopeUID(query.DatasourceUID))) { - return false - } - } - return true + return fmt.Sprintf("access one or many data sources%s", suffix) + }) +} + +// HasAccessToRuleGroup returns false if +func (r *RuleService) HasAccessToRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) (bool, error) { + eval := r.getRulesReadEvaluator(rules...) + return r.HasAccess(ctx, user, eval) } // AuthorizeAccessToRuleGroup checks all rules against AuthorizeDatasourceAccessForRule and exits on the first negative result -func (r *RuleService) AuthorizeAccessToRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) bool { - for _, rule := range rules { - if !r.AuthorizeDatasourceAccessForRule(ctx, user, rule) { - return false +func (r *RuleService) AuthorizeAccessToRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) error { + eval := r.getRulesReadEvaluator(rules...) + return r.HasAccessOrError(ctx, user, eval, func() string { + var groupName, folderUID string + if len(rules) > 0 { + groupName = rules[0].RuleGroup + folderUID = rules[0].NamespaceUID } - } - return true + return fmt.Sprintf("access rule group '%s' in folder '%s'", groupName, folderUID) + }) } // AuthorizeRuleChanges analyzes changes in the rule group, and checks whether the changes are authorized. // NOTE: if there are rules for deletion, and the user does not have access to data sources that a rule uses, the rule is removed from the list. // If the user is not authorized to perform the changes the function returns ErrAuthorization with a description of what action is not authorized. -// Return changes that the user is authorized to perform or ErrAuthorization func (r *RuleService) AuthorizeRuleChanges(ctx context.Context, user identity.Requester, change *store.GroupDelta) error { namespaceScope := dashboards.ScopeFoldersProvider.GetResourceScopeUID(change.GroupKey.NamespaceUID) rules, ok := change.AffectedGroups[change.GroupKey] if ok { // not ok can be when user creates a new rule group or moves existing alerts to a new group - if !r.AuthorizeAccessToRuleGroup(ctx, user, rules) { // if user is not authorized to do operation in the group that is being changed - return fmt.Errorf("%w to change group %s because it does not have access to one or many rules in this group", ErrAuthorization, change.GroupKey.RuleGroup) + if err := r.AuthorizeAccessToRuleGroup(ctx, user, rules); err != nil { // if user is not authorized to do operation in the group that is being changed + return err } } else if len(change.Delete) > 0 { // add a safeguard in the case of inconsistency. If user hit this then there is a bug in the calculating of changes struct @@ -86,54 +124,68 @@ func (r *RuleService) AuthorizeRuleChanges(ctx context.Context, user identity.Re } if len(change.Delete) > 0 { - allowed := r.HasAccess(ctx, user, accesscontrol.EvalPermission(ruleDelete, namespaceScope)) - if !allowed { - return fmt.Errorf("%w to delete alert rules that belong to folder %s", ErrAuthorization, change.GroupKey.NamespaceUID) + if err := r.HasAccessOrError(ctx, user, accesscontrol.EvalPermission(ruleDelete, namespaceScope), func() string { + return fmt.Sprintf("delete alert rules that belong to folder %s", change.GroupKey.NamespaceUID) + }); err != nil { + return err } for _, rule := range change.Delete { - if !r.AuthorizeDatasourceAccessForRule(ctx, user, rule) { - return fmt.Errorf("%w to delete an alert rule '%s' because the user does not have read permissions for one or many datasources the rule uses", ErrAuthorization, rule.UID) + if err := r.HasAccessOrError(ctx, user, r.getRulesQueryEvaluator(rule), func() string { + return fmt.Sprintf("delete an alert rule '%s'", rule.UID) + }); err != nil { + return err } } } - var addAuthorized, updateAuthorized bool - + var addAuthorized, updateAuthorized bool // these are needed to check authorization for the rule create\update only once if len(change.New) > 0 { - addAuthorized = r.HasAccess(ctx, user, accesscontrol.EvalPermission(ruleCreate, namespaceScope)) - if !addAuthorized { - return fmt.Errorf("%w to create alert rules in the folder %s", ErrAuthorization, change.GroupKey.NamespaceUID) + if err := r.HasAccessOrError(ctx, user, accesscontrol.EvalPermission(ruleCreate, namespaceScope), func() string { + return fmt.Sprintf("create alert rules in the folder %s", change.GroupKey.NamespaceUID) + }); err != nil { + return err } + addAuthorized = true for _, rule := range change.New { - if !r.AuthorizeDatasourceAccessForRule(ctx, user, rule) { - 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) + if err := r.HasAccessOrError(ctx, user, r.getRulesQueryEvaluator(rule), func() string { + return fmt.Sprintf("create a new alert rule '%s'", rule.Title) + }); err != nil { + return err } } } for _, rule := range change.Update { - if !r.AuthorizeDatasourceAccessForRule(ctx, user, rule.New) { - 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) + if err := r.HasAccessOrError(ctx, user, r.getRulesQueryEvaluator(rule.New), func() string { + return fmt.Sprintf("update alert rule '%s' (UID: %s)", rule.Existing.Title, rule.Existing.UID) + }); err != nil { + return err } // Check if the rule is moved from one folder to the current. If yes, then the user must have the authorization to delete rules from the source folder and add rules to the target folder. if rule.Existing.NamespaceUID != rule.New.NamespaceUID { - allowed := r.HasAccess(ctx, user, accesscontrol.EvalPermission(ruleDelete, dashboards.ScopeFoldersProvider.GetResourceScopeUID(rule.Existing.NamespaceUID))) - if !allowed { - return fmt.Errorf("%w to delete alert rules from folder UID %s", ErrAuthorization, rule.Existing.NamespaceUID) + ev := accesscontrol.EvalPermission(ruleDelete, dashboards.ScopeFoldersProvider.GetResourceScopeUID(rule.Existing.NamespaceUID)) + if err := r.HasAccessOrError(ctx, user, ev, func() string { + return fmt.Sprintf("move alert rules from folder %s", rule.Existing.NamespaceUID) + }); err != nil { + return err } if !addAuthorized { - addAuthorized = r.HasAccess(ctx, user, accesscontrol.EvalPermission(ruleCreate, namespaceScope)) - if !addAuthorized { - return fmt.Errorf("%w to create alert rules in the folder '%s'", ErrAuthorization, change.GroupKey.NamespaceUID) + if err := r.HasAccessOrError(ctx, user, accesscontrol.EvalPermission(ruleCreate, namespaceScope), func() string { + return fmt.Sprintf("move alert rules to folder '%s'", change.GroupKey.NamespaceUID) + }); err != nil { + return err } + addAuthorized = true } } else if !updateAuthorized { // if it is false then the authorization was not checked. If it is true then the user is authorized to update rules - updateAuthorized = r.HasAccess(ctx, user, accesscontrol.EvalPermission(ruleUpdate, namespaceScope)) - if !updateAuthorized { - return fmt.Errorf("%w to update alert rules that belong to folder %s", ErrAuthorization, change.GroupKey.NamespaceUID) + if err := r.HasAccessOrError(ctx, user, accesscontrol.EvalPermission(ruleUpdate, namespaceScope), func() string { + return fmt.Sprintf("update alert rules that belongs to folder '%s'", change.GroupKey.NamespaceUID) + }); err != nil { + return err } + updateAuthorized = true } if rule.Existing.NamespaceUID != rule.New.NamespaceUID || rule.Existing.RuleGroup != rule.New.RuleGroup { @@ -143,8 +195,10 @@ func (r *RuleService) AuthorizeRuleChanges(ctx context.Context, user identity.Re // add a safeguard in the case of inconsistency. If user hit this then there is a bug in the calculating of changes struct return fmt.Errorf("failed to authorize moving an alert rule %s between groups because unable to check access to group %s from which the rule is moved", rule.Existing.UID, rule.Existing.RuleGroup) } - if !r.AuthorizeAccessToRuleGroup(ctx, user, rules) { - return fmt.Errorf("%w to move rule %s between two different groups because user does not have access to the source group %s", ErrAuthorization, rule.Existing.UID, rule.Existing.RuleGroup) + if err := r.HasAccessOrError(ctx, user, r.getRulesQueryEvaluator(rules...), func() string { + return fmt.Sprintf("move rule %s between two different groups because user does not have access to the source group %s", rule.Existing.UID, rule.Existing.RuleGroup) + }); err != nil { + return err } } } diff --git a/pkg/services/ngalert/accesscontrol/rules_test.go b/pkg/services/ngalert/accesscontrol/rules_test.go index c05628bc41d..bd84328305d 100644 --- a/pkg/services/ngalert/accesscontrol/rules_test.go +++ b/pkg/services/ngalert/accesscontrol/rules_test.go @@ -6,6 +6,8 @@ import ( "math/rand" "testing" + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/expr" @@ -328,8 +330,8 @@ func TestAuthorizeRuleChanges(t *testing.T) { ac: ac, } err := srv.AuthorizeRuleChanges(context.Background(), createUserWithPermissions(missing), groupChanges) - require.Errorf(t, err, "expected error because less permissions than expected were provided. Provided: %v; Expected: %v", missing, permissions) - require.ErrorIs(t, err, ErrAuthorization) + + assert.Errorf(t, err, "expected error because less permissions than expected were provided. Provided: %v; Expected: %v; Diff: %v", missing, permissions, cmp.Diff(permissions, missing)) require.NotEmptyf(t, ac.EvaluateRecordings, "Access control was supposed to be called but it was not") } }) @@ -361,8 +363,7 @@ func TestCheckDatasourcePermissionsForRule(t *testing.T) { var data []models.AlertQuery var scopes []string - expectedExecutions := rand.Intn(3) + 2 - for i := 0; i < expectedExecutions; i++ { + for i := 0; i < rand.Intn(3)+2; i++ { q := models.GenerateAlertQuery() scopes = append(scopes, datasources.ScopeProvider.GetResourceScopeUID(q.DatasourceUID)) data = append(data, q) @@ -387,8 +388,8 @@ func TestCheckDatasourcePermissionsForRule(t *testing.T) { eval := svc.AuthorizeDatasourceAccessForRule(context.Background(), createUserWithPermissions(permissions), rule) - require.True(t, eval) - require.Len(t, ac.EvaluateRecordings, expectedExecutions) + require.NoError(t, eval) + require.Len(t, ac.EvaluateRecordings, 1) }) t.Run("should return on first negative evaluation", func(t *testing.T) { @@ -401,9 +402,9 @@ func TestCheckDatasourcePermissionsForRule(t *testing.T) { ac: ac, } - eval := svc.AuthorizeDatasourceAccessForRule(context.Background(), createUserWithPermissions(nil), rule) + result := svc.AuthorizeDatasourceAccessForRule(context.Background(), createUserWithPermissions(nil), rule) - require.False(t, eval) + require.Error(t, result) require.Len(t, ac.EvaluateRecordings, 1) }) } @@ -427,7 +428,7 @@ func Test_authorizeAccessToRuleGroup(t *testing.T) { result := svc.AuthorizeAccessToRuleGroup(context.Background(), createUserWithPermissions(permissions), rules) - require.True(t, result) + require.NoError(t, result) require.NotEmpty(t, ac.EvaluateRecordings) }) t.Run("should return false if user does not have access to at least one rule in group", func(t *testing.T) { @@ -453,6 +454,6 @@ func Test_authorizeAccessToRuleGroup(t *testing.T) { result := svc.AuthorizeAccessToRuleGroup(context.Background(), createUserWithPermissions(permissions), rules) - require.False(t, result) + require.Error(t, result) }) } diff --git a/pkg/services/ngalert/api/api.go b/pkg/services/ngalert/api/api.go index 7b755d9a6dc..c5584ba0e64 100644 --- a/pkg/services/ngalert/api/api.go +++ b/pkg/services/ngalert/api/api.go @@ -40,8 +40,10 @@ type AlertingStore interface { } type RuleAccessControlService interface { - AuthorizeAccessToRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) bool + HasAccessToRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) (bool, error) + AuthorizeAccessToRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) error AuthorizeRuleChanges(ctx context.Context, user identity.Requester, change *store.GroupDelta) error + AuthorizeDatasourceAccessForRule(ctx context.Context, user identity.Requester, rule *models.AlertRule) error } // API handlers. diff --git a/pkg/services/ngalert/api/api_prometheus.go b/pkg/services/ngalert/api/api_prometheus.go index 51c7f3630fd..db539626d8c 100644 --- a/pkg/services/ngalert/api/api_prometheus.go +++ b/pkg/services/ngalert/api/api_prometheus.go @@ -235,7 +235,11 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *contextmodel.ReqContext) respon srv.log.Warn("Query returned rules that belong to folder the user does not have access to. All rules that belong to that namespace will not be added to the response", "folder_uid", groupKey.NamespaceUID) continue } - if !srv.authz.AuthorizeAccessToRuleGroup(c.Req.Context(), c.SignedInUser, rules) { + ok, err := srv.authz.HasAccessToRuleGroup(c.Req.Context(), c.SignedInUser, rules) + if err != nil { + return response.ErrOrFallback(http.StatusInternalServerError, "cannot authorize access to rule group", err) + } + if !ok { continue } ruleGroup, totals := srv.toRuleGroup(groupKey, folder, rules, limitAlertsPerRule, withStatesFast, matchers, labelOptions) diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index 98aa94ad372..2f3d772ef32 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -25,6 +25,7 @@ import ( "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" + "github.com/grafana/grafana/pkg/util/errutil" ) type ConditionValidator interface { @@ -96,7 +97,7 @@ func (srv RulerSrv) RouteDeleteAlertRules(c *contextmodel.ReqContext, namespaceT return err } if totalGroups > 0 && len(deletionCandidates) == 0 { - return fmt.Errorf("%w to delete any existing rules in the namespace", accesscontrol.ErrAuthorization) + return accesscontrol.NewAuthorizationErrorGeneric("delete any existing rules in the namespace") } } rulesToDelete := make([]string, 0) @@ -131,8 +132,8 @@ func (srv RulerSrv) RouteDeleteAlertRules(c *contextmodel.ReqContext, namespaceT }) if err != nil { - if errors.Is(err, accesscontrol.ErrAuthorization) { - return ErrResp(http.StatusUnauthorized, err, "failed to delete rule group") + if errors.As(err, &errutil.Error{}) { + return response.Err(err) } if errors.Is(err, errProvisionedResource) { return ErrResp(http.StatusBadRequest, err, "failed to delete rule group") @@ -365,14 +366,14 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *contextmodel.ReqContext, groupKey }) if err != nil { - if errors.Is(err, ngmodels.ErrAlertRuleNotFound) { + if errors.As(err, &errutil.Error{}) { + return response.Err(err) + } else if errors.Is(err, ngmodels.ErrAlertRuleNotFound) { return ErrResp(http.StatusNotFound, err, "failed to update rule group") } else if errors.Is(err, ngmodels.ErrAlertRuleFailedValidation) || errors.Is(err, errProvisionedResource) { return ErrResp(http.StatusBadRequest, err, "failed to update rule group") } else if errors.Is(err, ngmodels.ErrQuotaReached) { return ErrResp(http.StatusForbidden, err, "") - } else if errors.Is(err, accesscontrol.ErrAuthorization) { - return ErrResp(http.StatusUnauthorized, err, "") } else if errors.Is(err, store.ErrOptimisticLock) { return ErrResp(http.StatusConflict, err, "") } @@ -521,8 +522,8 @@ func (srv RulerSrv) getAuthorizedRuleByUid(ctx context.Context, c *contextmodel. if err != nil { return ngmodels.AlertRule{}, err } - if !srv.authz.AuthorizeAccessToRuleGroup(ctx, c.SignedInUser, rules) { - return ngmodels.AlertRule{}, fmt.Errorf("%w to access rules in this group", accesscontrol.ErrAuthorization) + if err := srv.authz.AuthorizeAccessToRuleGroup(ctx, c.SignedInUser, rules); err != nil { + return ngmodels.AlertRule{}, err } for _, rule := range rules { if rule.UID == ruleUID { @@ -545,8 +546,8 @@ func (srv RulerSrv) getAuthorizedRuleGroup(ctx context.Context, c *contextmodel. if err != nil { return nil, err } - if !srv.authz.AuthorizeAccessToRuleGroup(ctx, c.SignedInUser, rules) { - return nil, fmt.Errorf("%w to access rules in this group", accesscontrol.ErrAuthorization) + if err := srv.authz.AuthorizeAccessToRuleGroup(ctx, c.SignedInUser, rules); err != nil { + return nil, err } return rules, nil } @@ -569,7 +570,10 @@ func (srv RulerSrv) searchAuthorizedAlertRules(ctx context.Context, c *contextmo byGroupKey := ngmodels.GroupByAlertRuleGroupKey(rules) totalGroups := len(byGroupKey) for groupKey, rulesGroup := range byGroupKey { - if !srv.authz.AuthorizeAccessToRuleGroup(ctx, c.SignedInUser, rulesGroup) { + if ok, err := srv.authz.HasAccessToRuleGroup(ctx, c.SignedInUser, rulesGroup); !ok || err != nil { + if err != nil { + return nil, 0, err + } delete(byGroupKey, groupKey) } } diff --git a/pkg/services/ngalert/api/api_ruler_export.go b/pkg/services/ngalert/api/api_ruler_export.go index ad895e7233d..2f030d89a87 100644 --- a/pkg/services/ngalert/api/api_ruler_export.go +++ b/pkg/services/ngalert/api/api_ruler_export.go @@ -148,7 +148,7 @@ func (srv RulerSrv) getRulesWithFolderTitleInFolders(c *contextmodel.ReqContext, } } if len(query.NamespaceUIDs) == 0 { - return nil, fmt.Errorf("%w access rules in the specified folders", accesscontrol.ErrAuthorization) + return nil, accesscontrol.NewAuthorizationErrorGeneric("access rules in the specified folders") } } else { for _, folder := range folders { diff --git a/pkg/services/ngalert/api/api_testing.go b/pkg/services/ngalert/api/api_testing.go index 70a4f89fc31..fa9b945dd46 100644 --- a/pkg/services/ngalert/api/api_testing.go +++ b/pkg/services/ngalert/api/api_testing.go @@ -2,7 +2,6 @@ package api import ( "errors" - "fmt" "net/http" "net/url" "strconv" @@ -21,7 +20,6 @@ import ( "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/folder" - "github.com/grafana/grafana/pkg/services/ngalert/accesscontrol" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" "github.com/grafana/grafana/pkg/services/ngalert/backtesting" "github.com/grafana/grafana/pkg/services/ngalert/eval" @@ -65,8 +63,8 @@ func (srv TestingApiSrv) RouteTestGrafanaRuleConfig(c *contextmodel.ReqContext, return ErrResp(http.StatusBadRequest, err, "") } - if !srv.authz.AuthorizeAccessToRuleGroup(c.Req.Context(), c.SignedInUser, ngmodels.RulesGroup{rule}) { - return errorToResponse(fmt.Errorf("%w to query one or many data sources used by the rule", accesscontrol.ErrAuthorization)) + if err := srv.authz.AuthorizeAccessToRuleGroup(c.Req.Context(), c.SignedInUser, ngmodels.RulesGroup{rule}); err != nil { + return response.ErrOrFallback(http.StatusInternalServerError, "failed to authorize access to rule group", err) } if _, err := store.OptimizeAlertQueries(rule.Data); err != nil { @@ -153,8 +151,8 @@ func (srv TestingApiSrv) RouteTestRuleConfig(c *contextmodel.ReqContext, body ap func (srv TestingApiSrv) RouteEvalQueries(c *contextmodel.ReqContext, cmd apimodels.EvalQueriesPayload) response.Response { queries := AlertQueriesFromApiAlertQueries(cmd.Data) - if !srv.authz.AuthorizeAccessToRuleGroup(c.Req.Context(), c.SignedInUser, ngmodels.RulesGroup{&ngmodels.AlertRule{Data: queries}}) { - return ErrResp(http.StatusUnauthorized, fmt.Errorf("%w to query one or many data sources used by the rule", accesscontrol.ErrAuthorization), "") + if err := srv.authz.AuthorizeDatasourceAccessForRule(c.Req.Context(), c.SignedInUser, &ngmodels.AlertRule{Data: queries}); err != nil { + return response.ErrOrFallback(http.StatusInternalServerError, "failed to authorize access to data sources", err) } cond := ngmodels.Condition{ @@ -215,8 +213,8 @@ func (srv TestingApiSrv) BacktestAlertRule(c *contextmodel.ReqContext, cmd apimo } queries := AlertQueriesFromApiAlertQueries(cmd.Data) - if !srv.authz.AuthorizeAccessToRuleGroup(c.Req.Context(), c.SignedInUser, ngmodels.RulesGroup{&ngmodels.AlertRule{Data: queries}}) { - return errorToResponse(fmt.Errorf("%w to query one or many data sources used by the rule", accesscontrol.ErrAuthorization)) + if err := srv.authz.AuthorizeAccessToRuleGroup(c.Req.Context(), c.SignedInUser, ngmodels.RulesGroup{&ngmodels.AlertRule{Data: queries}}); err != nil { + return errorToResponse(err) } rule := &ngmodels.AlertRule{ diff --git a/pkg/services/ngalert/api/errors.go b/pkg/services/ngalert/api/errors.go index 7fe54f0b5bb..1e9dd272b24 100644 --- a/pkg/services/ngalert/api/errors.go +++ b/pkg/services/ngalert/api/errors.go @@ -6,8 +6,8 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/services/datasources" - "github.com/grafana/grafana/pkg/services/ngalert/accesscontrol" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" + "github.com/grafana/grafana/pkg/util/errutil" ) var ( @@ -29,15 +29,15 @@ func backendTypeDoesNotMatchPayloadTypeError(backendType apimodels.Backend, payl } func errorToResponse(err error) response.Response { + if errors.As(err, &errutil.Error{}) { + return response.Err(err) + } if errors.Is(err, datasources.ErrDataSourceNotFound) { return ErrResp(404, err, "") } if errors.Is(err, errUnexpectedDatasourceType) { return ErrResp(400, err, "") } - if errors.Is(err, accesscontrol.ErrAuthorization) { - return ErrResp(401, err, "") - } if errors.Is(err, errFolderAccess) { return toNamespaceErrorResponse(err) } diff --git a/pkg/services/ngalert/api/testing.go b/pkg/services/ngalert/api/testing.go index 4fecc5d0f1a..2e9b2cee6e0 100644 --- a/pkg/services/ngalert/api/testing.go +++ b/pkg/services/ngalert/api/testing.go @@ -145,10 +145,18 @@ var _ accesscontrol.AccessControl = &recordingAccessControlFake{} type fakeRuleAccessControlService struct { } -func (f fakeRuleAccessControlService) AuthorizeAccessToRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) bool { - return true +func (f fakeRuleAccessControlService) HasAccessToRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) (bool, error) { + return true, nil +} + +func (f fakeRuleAccessControlService) AuthorizeAccessToRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) error { + return nil } func (f fakeRuleAccessControlService) AuthorizeRuleChanges(ctx context.Context, user identity.Requester, change *store.GroupDelta) error { return nil } + +func (f fakeRuleAccessControlService) AuthorizeDatasourceAccessForRule(ctx context.Context, user identity.Requester, rule *models.AlertRule) error { + return nil +} diff --git a/pkg/tests/api/alerting/api_alertmanager_test.go b/pkg/tests/api/alerting/api_alertmanager_test.go index 7b9f1268316..fb394eb8692 100644 --- a/pkg/tests/api/alerting/api_alertmanager_test.go +++ b/pkg/tests/api/alerting/api_alertmanager_test.go @@ -1051,7 +1051,7 @@ func TestIntegrationAlertRuleCRUD(t *testing.T) { }(), expectedMessage: func() string { if setting.IsEnterprise { - return "user is not authorized to create a new alert rule 'AlwaysFiring' because the user does not have read permissions for one or many datasources the rule uses" + return "user is not authorized to create a new alert rule 'AlwaysFiring'" } return "failed to update rule group: invalid alert rule 'AlwaysFiring': failed to build query 'A': data source not found" }(), @@ -2284,7 +2284,7 @@ func TestIntegrationEval(t *testing.T) { }, expectedMessage: func() string { if setting.IsEnterprise { - return "user is not authorized to query one or many data sources used by the rule" + return "user is not authorized to access one or many data sources" } return "Failed to build evaluator for queries and expressions: failed to build query 'A': data source not found" }, diff --git a/pkg/tests/api/alerting/api_backtesting_test.go b/pkg/tests/api/alerting/api_backtesting_test.go index 286245fa500..f62b70870ce 100644 --- a/pkg/tests/api/alerting/api_backtesting_test.go +++ b/pkg/tests/api/alerting/api_backtesting_test.go @@ -18,7 +18,6 @@ import ( apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" - "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/tests/testinfra" ) @@ -92,10 +91,6 @@ func TestBacktesting(t *testing.T) { }) t.Run("if user does not have permissions", func(t *testing.T) { - if !setting.IsEnterprise { - t.Skip("Enterprise-only test") - } - testUserId := createUser(t, env.SQLStore, user.CreateUserCommand{ DefaultOrgRole: "", Password: "test", @@ -128,7 +123,7 @@ func TestBacktesting(t *testing.T) { t.Run("fail if can't query data sources", func(t *testing.T) { status, body := testUserApiCli.SubmitRuleForBacktesting(t, queryRequest) - require.Contains(t, body, "user is not authorized to query one or many data sources used by the rule") + require.Contains(t, body, "user is not authorized to access rule group") require.Equalf(t, http.StatusUnauthorized, status, "Response: %s", body) }) })