-
Notifications
You must be signed in to change notification settings - Fork 11.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Alerting: Update rule access control to return errutil errors #78284
Conversation
that's what accesscontrol middleware uses
return false | ||
// 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...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add more in a follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far, going to do some manual testing but I'll post my code comments for now.
}) | ||
} | ||
|
||
// HasAccessToRuleGroup returns false if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing some text in the comment
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Should this be errorToResponse(err)
as well to be inline with the rest of the api?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I think it is better to do the opposite because ErrOrFallback lets us return some meaningful message in the response if error does not happen to be errutils' one.
This could be useful if authz happen to return some generic errors (eg from db or other service).
WDYT?
@@ -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 rule group in folder " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's quote the rule group name and folder to avoid this odd double space in
return fmt.Sprintf("access rule group %s in folder %s", groupName, folderUID) |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this wasn't technically changed in this PR, but could consider using query.IsExpression()
here. Could even do a very small refactor to remove the unused error
return so it can be inlined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather get rid of the query.IsExpression
because it makes a dependency on expr
package, and keep the logic that binds expr
and ng.models
packages in 3rd package.
This check is not necessary, though, and can be replaced by similar logic as
grafana/pkg/services/ngalert/eval/eval.go
Lines 298 to 307 in 580477b
switch nodeType := expr.NodeTypeFromDatasourceUID(q.DatasourceUID); nodeType { | |
case expr.TypeDatasourceNode: | |
ds, err = dsCacheService.GetDatasourceByUID(ctx.Ctx, q.DatasourceUID, ctx.User, false /*skipCache*/) | |
default: | |
ds, err = expr.DataSourceModelFromNodeType(nodeType) | |
} | |
if err != nil { | |
return nil, fmt.Errorf("failed to build query '%s': %w", q.RefID, err) | |
} | |
datasources[q.DatasourceUID] = ds |
to facilitate checks for MLNode
in the future.
I would like to keep it as is to reduce the diff, and address in a follow-up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
What is this feature?
This PR updates the rule access control service to return errutil.Error errors that are used throughout the Grafana code.
Notable changes:
AuthorizeDatasourceAccessForRule
andAuthorizeAccessToRuleGroup
now collect all data source scopes and evaluate permissions once.AuthorizeRuleChanges
changed to evaluate permissions on its own using a new method HasAccessOrError. This opens the possibility for better messaging.Now the authorization error response will look like
Why do we need this feature?
Special notes for your reviewer:
Please check that: