From 006d0021e30d51981f988eb7075744ac55c0fc2c Mon Sep 17 00:00:00 2001 From: William Wernert Date: Thu, 23 May 2024 12:44:30 -0400 Subject: [PATCH] Alerting: Remove requirement for datasource query on rule read (#87349) * Remove requirement for datasource query for rule read * Address PR comments --- .../ngalert/accesscontrol/fakes/rules.go | 21 +++-- pkg/services/ngalert/accesscontrol/rules.go | 27 ++---- .../ngalert/accesscontrol/rules_test.go | 76 ++++------------- pkg/services/ngalert/api/api.go | 1 + pkg/services/ngalert/api/api_ruler.go | 60 +++++++++++--- pkg/services/ngalert/api/api_ruler_export.go | 10 ++- .../ngalert/api/api_ruler_export_test.go | 32 +++----- pkg/services/ngalert/api/api_ruler_test.go | 82 +++---------------- pkg/services/ngalert/api/testing.go | 4 + 9 files changed, 123 insertions(+), 190 deletions(-) diff --git a/pkg/services/ngalert/accesscontrol/fakes/rules.go b/pkg/services/ngalert/accesscontrol/fakes/rules.go index 0514a69cb9a..d7ddcda79a7 100644 --- a/pkg/services/ngalert/accesscontrol/fakes/rules.go +++ b/pkg/services/ngalert/accesscontrol/fakes/rules.go @@ -15,12 +15,13 @@ type Call struct { } type FakeRuleService struct { - HasAccessFunc func(context.Context, identity.Requester, accesscontrol.Evaluator) (bool, error) - HasAccessOrErrorFunc func(context.Context, identity.Requester, accesscontrol.Evaluator, func() string) error - AuthorizeDatasourceAccessForRuleFunc func(context.Context, identity.Requester, *models.AlertRule) error - HasAccessToRuleGroupFunc func(context.Context, identity.Requester, models.RulesGroup) (bool, error) - AuthorizeAccessToRuleGroupFunc func(context.Context, identity.Requester, models.RulesGroup) error - AuthorizeRuleChangesFunc func(context.Context, identity.Requester, *store.GroupDelta) error + HasAccessFunc func(context.Context, identity.Requester, accesscontrol.Evaluator) (bool, error) + HasAccessOrErrorFunc func(context.Context, identity.Requester, accesscontrol.Evaluator, func() string) error + AuthorizeDatasourceAccessForRuleFunc func(context.Context, identity.Requester, *models.AlertRule) error + AuthorizeDatasourceAccessForRuleGroupFunc func(context.Context, identity.Requester, models.RulesGroup) error + HasAccessToRuleGroupFunc func(context.Context, identity.Requester, models.RulesGroup) (bool, error) + AuthorizeAccessToRuleGroupFunc func(context.Context, identity.Requester, models.RulesGroup) error + AuthorizeRuleChangesFunc func(context.Context, identity.Requester, *store.GroupDelta) error Calls []Call } @@ -49,6 +50,14 @@ func (s *FakeRuleService) AuthorizeDatasourceAccessForRule(ctx context.Context, return nil } +func (s *FakeRuleService) AuthorizeDatasourceAccessForRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) error { + s.Calls = append(s.Calls, Call{"AuthorizeDatasourceAccessForRuleGroup", []interface{}{ctx, user, rules}}) + if s.AuthorizeDatasourceAccessForRuleGroupFunc != nil { + return s.AuthorizeDatasourceAccessForRuleGroupFunc(ctx, user, rules) + } + return nil +} + func (s *FakeRuleService) HasAccessToRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) (bool, error) { s.Calls = append(s.Calls, Call{"HasAccessToRuleGroup", []interface{}{ctx, user, rules}}) if s.HasAccessToRuleGroupFunc != nil { diff --git a/pkg/services/ngalert/accesscontrol/rules.go b/pkg/services/ngalert/accesscontrol/rules.go index b135cb6ac7d..8bb0769b1ef 100644 --- a/pkg/services/ngalert/accesscontrol/rules.go +++ b/pkg/services/ngalert/accesscontrol/rules.go @@ -50,8 +50,7 @@ func (r *RuleService) getRulesReadEvaluator(rules ...*models.AlertRule) accessco added[rule.NamespaceUID] = struct{}{} evals = append(evals, getReadFolderAccessEvaluator(rule.NamespaceUID)) } - dsEvals := r.getRulesQueryEvaluator(rules...) - return accesscontrol.EvalAll(append(evals, dsEvals)...) + return accesscontrol.EvalAll(evals...) } // getRulesQueryEvaluator constructs accesscontrol.Evaluator that checks all permissions to query data sources used by the provided rules @@ -89,10 +88,17 @@ func (r *RuleService) AuthorizeDatasourceAccessForRule(ctx context.Context, user }) } +// AuthorizeDatasourceAccessForRuleGroup checks that user has access to all data sources declared by the rules in the group +func (r *RuleService) AuthorizeDatasourceAccessForRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) error { + ds := r.getRulesQueryEvaluator(rules...) + return r.HasAccessOrError(ctx, user, ds, func() string { + return fmt.Sprintf("access data sources for the rule group '%s'", rules[0].RuleGroup) + }) +} + // AuthorizeAccessToRuleGroup checks that the identity.Requester has permissions to all rules, which means that it has permissions to: // - ("folders:read") read folders which contain the rules // - ("alert.rules:read") read alert rules in the folders -// - ("datasources:query") query all data sources that rules refer to // Returns false if the requester does not have enough permissions, and error if something went wrong during the permission evaluation. func (r *RuleService) HasAccessToRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) (bool, error) { eval := r.getRulesReadEvaluator(rules...) @@ -102,7 +108,6 @@ func (r *RuleService) HasAccessToRuleGroup(ctx context.Context, user identity.Re // AuthorizeAccessToRuleGroup checks that the identity.Requester has permissions to all rules, which means that it has permissions to: // - ("folders:read") read folders which contain the rules // - ("alert.rules:read") read alert rules in the folders -// - ("datasources:query") query all data sources that rules refer to // Returns error if at least one permissions is missing or if something went wrong during the permission evaluation func (r *RuleService) AuthorizeAccessToRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) error { eval := r.getRulesReadEvaluator(rules...) @@ -202,20 +207,6 @@ func (r *RuleService) AuthorizeRuleChanges(ctx context.Context, user identity.Re } updateAuthorized = true } - - if rule.Existing.NamespaceUID != rule.New.NamespaceUID || rule.Existing.RuleGroup != rule.New.RuleGroup { - key := rule.Existing.GetGroupKey() - rules, existingGroup = change.AffectedGroups[key] - if !existingGroup { - // 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 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 - } - } } return nil } diff --git a/pkg/services/ngalert/accesscontrol/rules_test.go b/pkg/services/ngalert/accesscontrol/rules_test.go index d426d0ddd90..43c94073089 100644 --- a/pkg/services/ngalert/accesscontrol/rules_test.go +++ b/pkg/services/ngalert/accesscontrol/rules_test.go @@ -157,7 +157,7 @@ func TestAuthorizeRuleChanges(t *testing.T) { ruleDelete: { namespaceIdScope, }, - datasources.ActionQuery: getDatasourceScopesForRules(c.AffectedGroups[c.GroupKey]), + datasources.ActionQuery: getDatasourceScopesForRules(c.Delete), } }, }, @@ -189,9 +189,9 @@ func TestAuthorizeRuleChanges(t *testing.T) { } }, permissions: func(c *store.GroupDelta) map[string][]string { - scopes := getDatasourceScopesForRules(append(c.AffectedGroups[c.GroupKey], mapUpdates(c.Update, func(update store.RuleDelta) *models.AlertRule { + scopes := getDatasourceScopesForRules(mapUpdates(c.Update, func(update store.RuleDelta) *models.AlertRule { return update.New - })...)) + })) return map[string][]string{ ruleRead: { namespaceIdScope, @@ -236,13 +236,9 @@ func TestAuthorizeRuleChanges(t *testing.T) { }, permissions: func(c *store.GroupDelta) map[string][]string { dsScopes := getDatasourceScopesForRules( - append(append(append(c.AffectedGroups[c.GroupKey], - mapUpdates(c.Update, func(update store.RuleDelta) *models.AlertRule { - return update.New - })..., - ), mapUpdates(c.Update, func(update store.RuleDelta) *models.AlertRule { - return update.Existing - })...), c.AffectedGroups[groupKey]...), + mapUpdates(c.Update, func(update store.RuleDelta) *models.AlertRule { + return update.New + }), ) var deleteScopes []string @@ -297,27 +293,11 @@ func TestAuthorizeRuleChanges(t *testing.T) { } }, permissions: func(c *store.GroupDelta) map[string][]string { - scopes := make(map[string]struct{}) - for _, update := range c.Update { - for _, query := range update.New.Data { - scopes[datasources.ScopeProvider.GetResourceScopeUID(query.DatasourceUID)] = struct{}{} - } - for _, query := range update.Existing.Data { - scopes[datasources.ScopeProvider.GetResourceScopeUID(query.DatasourceUID)] = struct{}{} - } - } - for _, rules := range c.AffectedGroups { - for _, rule := range rules { - for _, query := range rule.Data { - scopes[datasources.ScopeProvider.GetResourceScopeUID(query.DatasourceUID)] = struct{}{} - } - } - } - - dsScopes := make([]string, 0, len(scopes)) - for key := range scopes { - dsScopes = append(dsScopes, key) - } + dsScopes := getDatasourceScopesForRules( + mapUpdates(c.Update, func(update store.RuleDelta) *models.AlertRule { + return update.New + }), + ) return map[string][]string{ ruleRead: { @@ -435,14 +415,8 @@ func TestCheckDatasourcePermissionsForRule(t *testing.T) { } func Test_authorizeAccessToRuleGroup(t *testing.T) { - t.Run("should return true if user has access to all datasources of all rules in group", func(t *testing.T) { + t.Run("should succeed if user has access to all namespaces", func(t *testing.T) { rules := models.RuleGen.GenerateManyRef(1, 5) - var scopes []string - for _, rule := range rules { - for _, query := range rule.Data { - scopes = append(scopes, datasources.ScopeProvider.GetResourceScopeUID(query.DatasourceUID)) - } - } namespaceScopes := make([]string, 0) for _, rule := range rules { namespaceScopes = append(namespaceScopes, dashboards.ScopeFoldersProvider.GetResourceScopeUID(rule.NamespaceUID)) @@ -450,7 +424,6 @@ func Test_authorizeAccessToRuleGroup(t *testing.T) { permissions := map[string][]string{ ruleRead: namespaceScopes, dashboards.ActionFoldersRead: namespaceScopes, - datasources.ActionQuery: scopes, } ac := &recordingAccessControlFake{} svc := RuleService{ @@ -462,38 +435,21 @@ func Test_authorizeAccessToRuleGroup(t *testing.T) { 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) { + + t.Run("should fail if user does not have access to namespace", func(t *testing.T) { f := &folder.Folder{UID: "test-folder"} gen := models.RuleGen genWithFolder := gen.With(gen.WithNamespace(f)) rules := genWithFolder.GenerateManyRef(1, 5) - var scopes []string - for _, rule := range rules { - for _, query := range rule.Data { - scopes = append(scopes, datasources.ScopeProvider.GetResourceScopeUID(query.DatasourceUID)) - } - } - permissions := map[string][]string{ - ruleRead: { - dashboards.ScopeFoldersProvider.GetResourceScopeUID(f.UID), - }, - dashboards.ActionFoldersRead: { - dashboards.ScopeFoldersProvider.GetResourceScopeUID(f.UID), - }, - datasources.ActionQuery: scopes, - } - - rule := genWithFolder.GenerateRef() - rules = append(rules, rule) ac := &recordingAccessControlFake{} - svc := RuleService{ genericService{ac: ac}, } - result := svc.AuthorizeAccessToRuleGroup(context.Background(), createUserWithPermissions(permissions), rules) + result := svc.AuthorizeAccessToRuleGroup(context.Background(), createUserWithPermissions(map[string][]string{}), rules) require.Error(t, result) + require.ErrorIs(t, result, ErrAuthorizationBase) }) } diff --git a/pkg/services/ngalert/api/api.go b/pkg/services/ngalert/api/api.go index 8f6cf4ab502..16268df7e1a 100644 --- a/pkg/services/ngalert/api/api.go +++ b/pkg/services/ngalert/api/api.go @@ -44,6 +44,7 @@ type RuleAccessControlService interface { 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 + AuthorizeDatasourceAccessForRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) error } // API handlers. diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index 90def1eb650..5a0142be735 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -18,7 +18,7 @@ import ( contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/featuremgmt" - "github.com/grafana/grafana/pkg/services/ngalert/accesscontrol" + authz "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/eval" ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" @@ -110,22 +110,36 @@ func (srv RulerSrv) RouteDeleteAlertRules(c *contextmodel.ReqContext, namespaceU deletionCandidates[key] = rules } else { var totalGroups int - deletionCandidates, totalGroups, err = srv.searchAuthorizedAlertRules(ctx, c, []string{namespace.UID}, "", 0) + deletionCandidates, totalGroups, err = srv.searchAuthorizedAlertRules(ctx, authorizedRuleGroupQuery{ + User: c.SignedInUser, + NamespaceUIDs: []string{namespace.UID}, + }) if err != nil { return err } if totalGroups > 0 && len(deletionCandidates) == 0 { - return accesscontrol.NewAuthorizationErrorGeneric("delete any existing rules in the namespace") + return authz.NewAuthorizationErrorGeneric("delete any existing rules in the namespace due to missing data source query permissions") } } rulesToDelete := make([]string, 0) provisioned := false + auth := true for groupKey, rules := range deletionCandidates { if containsProvisionedAlerts(provenances, rules) { logger.Debug("Alert group cannot be deleted because it is provisioned", "group", groupKey.RuleGroup) provisioned = true continue } + // XXX: Currently delete requires data source query access to all rules in the group. + if err := srv.authz.AuthorizeDatasourceAccessForRuleGroup(ctx, c.SignedInUser, rules); err != nil { + if errors.Is(err, authz.ErrAuthorizationBase) { + logger.Debug("User is not authorized to delete rules in the group", "group", groupKey.RuleGroup) + auth = false + continue + } else { + return err + } + } uid := make([]string, 0, len(rules)) for _, rule := range rules { uid = append(uid, rule.UID) @@ -141,10 +155,17 @@ func (srv RulerSrv) RouteDeleteAlertRules(c *contextmodel.ReqContext, namespaceU return nil } // if none rules were deleted return an error. + // Check whether provisioned check failed first because if it is true, then all rules that the user can access (actually read via GET API) are provisioned. if provisioned { return errProvisionedResource } + + // If auth is false, then the user is not authorized to delete any of the rules. + if !auth { + return authz.NewAuthorizationErrorGeneric("delete any existing rules in the namespace") + } + logger.Info("No alert rules were deleted") return nil }) @@ -168,7 +189,10 @@ func (srv RulerSrv) RouteGetNamespaceRulesConfig(c *contextmodel.ReqContext, nam return toNamespaceErrorResponse(err) } - ruleGroups, _, err := srv.searchAuthorizedAlertRules(c.Req.Context(), c, []string{namespace.UID}, "", 0) + ruleGroups, _, err := srv.searchAuthorizedAlertRules(c.Req.Context(), authorizedRuleGroupQuery{ + User: c.SignedInUser, + NamespaceUIDs: []string{namespace.UID}, + }) if err != nil { return errorToResponse(err) } @@ -242,7 +266,12 @@ func (srv RulerSrv) RouteGetRulesConfig(c *contextmodel.ReqContext) response.Res return ErrResp(http.StatusBadRequest, errors.New("panel_id must be set with dashboard_uid"), "") } - configs, _, err := srv.searchAuthorizedAlertRules(c.Req.Context(), c, namespaceUIDs, dashboardUID, panelID) + configs, _, err := srv.searchAuthorizedAlertRules(c.Req.Context(), authorizedRuleGroupQuery{ + User: c.SignedInUser, + NamespaceUIDs: namespaceUIDs, + DashboardUID: dashboardUID, + PanelID: panelID, + }) if err != nil { return errorToResponse(err) } @@ -635,7 +664,6 @@ func (srv RulerSrv) getAuthorizedRuleByUid(ctx context.Context, c *contextmodel. } // getAuthorizedRuleGroup fetches rules that belong to the specified models.AlertRuleGroupKey and validate user's authorization. -// A user is authorized to access a group of rules only when it has permission to query all data sources used by all rules in this group. // Returns models.RuleGroup if authorization passed or ErrAuthorization if user is not authorized to access the rule. func (srv RulerSrv) getAuthorizedRuleGroup(ctx context.Context, c *contextmodel.ReqContext, ruleGroupKey ngmodels.AlertRuleGroupKey) (ngmodels.RulesGroup, error) { q := ngmodels.ListAlertRulesQuery{ @@ -653,15 +681,21 @@ func (srv RulerSrv) getAuthorizedRuleGroup(ctx context.Context, c *contextmodel. return rules, nil } +type authorizedRuleGroupQuery struct { + User identity.Requester + NamespaceUIDs []string + DashboardUID string + PanelID int64 +} + // searchAuthorizedAlertRules fetches rules according to the filters, groups them by models.AlertRuleGroupKey and filters out groups that the current user is not authorized to access. -// A user is authorized to access a group of rules only when it has permission to query all data sources used by all rules in this group. // Returns groups that user is authorized to access, and total count of groups returned by query -func (srv RulerSrv) searchAuthorizedAlertRules(ctx context.Context, c *contextmodel.ReqContext, folderUIDs []string, dashboardUID string, panelID int64) (map[ngmodels.AlertRuleGroupKey]ngmodels.RulesGroup, int, error) { +func (srv RulerSrv) searchAuthorizedAlertRules(ctx context.Context, q authorizedRuleGroupQuery) (map[ngmodels.AlertRuleGroupKey]ngmodels.RulesGroup, int, error) { query := ngmodels.ListAlertRulesQuery{ - OrgID: c.SignedInUser.GetOrgID(), - NamespaceUIDs: folderUIDs, - DashboardUID: dashboardUID, - PanelID: panelID, + OrgID: q.User.GetOrgID(), + NamespaceUIDs: q.NamespaceUIDs, + DashboardUID: q.DashboardUID, + PanelID: q.PanelID, } rules, err := srv.store.ListAlertRules(ctx, &query) if err != nil { @@ -671,7 +705,7 @@ func (srv RulerSrv) searchAuthorizedAlertRules(ctx context.Context, c *contextmo byGroupKey := ngmodels.GroupByAlertRuleGroupKey(rules) totalGroups := len(byGroupKey) for groupKey, rulesGroup := range byGroupKey { - if ok, err := srv.authz.HasAccessToRuleGroup(ctx, c.SignedInUser, rulesGroup); !ok || err != nil { + if ok, err := srv.authz.HasAccessToRuleGroup(ctx, q.User, rulesGroup); !ok || err != nil { if err != nil { return nil, 0, err } diff --git a/pkg/services/ngalert/api/api_ruler_export.go b/pkg/services/ngalert/api/api_ruler_export.go index 12a916ad627..56d7d154f99 100644 --- a/pkg/services/ngalert/api/api_ruler_export.go +++ b/pkg/services/ngalert/api/api_ruler_export.go @@ -7,7 +7,8 @@ import ( "github.com/grafana/grafana/pkg/api/response" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" - "github.com/grafana/grafana/pkg/services/ngalert/accesscontrol" + + authz "github.com/grafana/grafana/pkg/services/ngalert/accesscontrol" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" ) @@ -148,7 +149,7 @@ func (srv RulerSrv) getRulesWithFolderTitleInFolders(c *contextmodel.ReqContext, } } if len(query.NamespaceUIDs) == 0 { - return nil, accesscontrol.NewAuthorizationErrorGeneric("access rules in the specified folders") + return nil, authz.NewAuthorizationErrorGeneric("access rules in the specified folders") } } else { for _, folder := range folders { @@ -156,7 +157,10 @@ func (srv RulerSrv) getRulesWithFolderTitleInFolders(c *contextmodel.ReqContext, } } - rulesByGroup, _, err := srv.searchAuthorizedAlertRules(c.Req.Context(), c, folderUIDs, "", 0) + rulesByGroup, _, err := srv.searchAuthorizedAlertRules(c.Req.Context(), authorizedRuleGroupQuery{ + User: c.SignedInUser, + NamespaceUIDs: folderUIDs, + }) if err != nil { return nil, err } diff --git a/pkg/services/ngalert/api/api_ruler_export_test.go b/pkg/services/ngalert/api/api_ruler_export_test.go index 665daa4441b..fcc2783bc4b 100644 --- a/pkg/services/ngalert/api/api_ruler_export_test.go +++ b/pkg/services/ngalert/api/api_ruler_export_test.go @@ -253,6 +253,11 @@ func TestExportRules(t *testing.T) { srv := createService(ruleStore) + allRules := make([]*ngmodels.AlertRule, 0, len(hasAccess1)+len(hasAccess2)+len(noAccess1)) + allRules = append(allRules, hasAccess1...) + allRules = append(allRules, hasAccess2...) + allRules = append(allRules, noAccess1...) + testCases := []struct { title string params url.Values @@ -267,7 +272,7 @@ func TestExportRules(t *testing.T) { expectedHeaders: http.Header{ "Content-Type": []string{"text/yaml"}, }, - expectedRules: append(hasAccess1, hasAccess2...), + expectedRules: allRules, }, { title: "return all rules in folder", @@ -278,7 +283,7 @@ func TestExportRules(t *testing.T) { expectedHeaders: http.Header{ "Content-Type": []string{"text/yaml"}, }, - expectedRules: hasAccess1, + expectedRules: append(hasAccess1, noAccess1...), }, { title: "return all rules in many folders", @@ -289,7 +294,7 @@ func TestExportRules(t *testing.T) { expectedHeaders: http.Header{ "Content-Type": []string{"text/yaml"}, }, - expectedRules: append(hasAccess1, hasAccess2...), + expectedRules: allRules, }, { title: "return rules in single group", @@ -347,28 +352,13 @@ func TestExportRules(t *testing.T) { expectedStatus: http.StatusForbidden, expectedRules: nil, }, - { - title: "forbidden if group is not accessible", - params: url.Values{ - "folderUid": []string{noAccessKey1.NamespaceUID}, - "group": []string{noAccessKey1.RuleGroup}, - }, - expectedStatus: http.StatusForbidden, - }, - { - title: "forbidden if rule's group is not accessible", - params: url.Values{ - "ruleUid": []string{noAccessRule.UID}, - }, - expectedStatus: http.StatusForbidden, - }, { title: "return in JSON if header is specified", headers: http.Header{ "Accept": []string{"application/json"}, }, expectedStatus: 200, - expectedRules: append(hasAccess1, hasAccess2...), + expectedRules: allRules, expectedHeaders: http.Header{ "Content-Type": []string{"application/json"}, }, @@ -379,7 +369,7 @@ func TestExportRules(t *testing.T) { "format": []string{"json"}, }, expectedStatus: 200, - expectedRules: append(hasAccess1, hasAccess2...), + expectedRules: allRules, expectedHeaders: http.Header{ "Content-Type": []string{"application/json"}, }, @@ -390,7 +380,7 @@ func TestExportRules(t *testing.T) { "format": []string{"hcl"}, }, expectedStatus: 200, - expectedRules: append(hasAccess1, hasAccess2...), + expectedRules: allRules, expectedHeaders: http.Header{ "Content-Type": []string{"text/hcl"}, }, diff --git a/pkg/services/ngalert/api/api_ruler_test.go b/pkg/services/ngalert/api/api_ruler_test.go index a1814cc3181..2b675426b2b 100644 --- a/pkg/services/ngalert/api/api_ruler_test.go +++ b/pkg/services/ngalert/api/api_ruler_test.go @@ -196,17 +196,22 @@ func TestRouteDeleteAlertRules(t *testing.T) { func TestRouteGetNamespaceRulesConfig(t *testing.T) { gen := models.RuleGen t.Run("fine-grained access is enabled", func(t *testing.T) { - t.Run("should return rules for which user has access to data source", func(t *testing.T) { + t.Run("should return all rules, with or without data source access", func(t *testing.T) { orgID := rand.Int63() folder := randFolder() ruleStore := fakes.NewRuleStore(t) ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) folderGen := gen.With(gen.WithOrgID(orgID), gen.WithNamespace(folder)) - expectedRules := folderGen.GenerateManyRef(2, 6) - ruleStore.PutRule(context.Background(), expectedRules...) - ruleStore.PutRule(context.Background(), folderGen.GenerateManyRef(2, 6)...) + queryAccessRules := folderGen.GenerateManyRef(2, 6) + ruleStore.PutRule(context.Background(), queryAccessRules...) + noQueryAccessRules := folderGen.GenerateManyRef(2, 6) + ruleStore.PutRule(context.Background(), noQueryAccessRules...) - permissions := createPermissionsForRules(expectedRules, orgID) + allRules := make([]*models.AlertRule, 0, len(queryAccessRules)+len(noQueryAccessRules)) + allRules = append(allRules, queryAccessRules...) + allRules = append(allRules, noQueryAccessRules...) + + permissions := createPermissionsForRules(queryAccessRules, orgID) req := createRequestContextWithPerms(orgID, permissions, nil) response := createService(ruleStore).RouteGetNamespaceRulesConfig(req, folder.UID) @@ -220,9 +225,9 @@ func TestRouteGetNamespaceRulesConfig(t *testing.T) { for _, group := range groups { grouploop: for _, actualRule := range group.Rules { - for i, expected := range expectedRules { + for i, expected := range allRules { if actualRule.GrafanaManagedAlert.UID == expected.UID { - expectedRules = append(expectedRules[:i], expectedRules[i+1:]...) + allRules = append(allRules[:i], allRules[i+1:]...) continue grouploop } } @@ -230,7 +235,7 @@ func TestRouteGetNamespaceRulesConfig(t *testing.T) { } } } - assert.Emptyf(t, expectedRules, "not all expected rules were returned") + assert.Emptyf(t, allRules, "not all expected rules were returned") }) }) t.Run("should return the provenance of the alert rules", func(t *testing.T) { @@ -367,28 +372,6 @@ func TestRouteGetRuleByUID(t *testing.T) { require.Equal(t, http.StatusNotFound, response.Status()) }) - - t.Run("error due to user not being authorized to view a rule in the group", func(t *testing.T) { - orgID := rand.Int63() - folder := randFolder() - ruleStore := fakes.NewRuleStore(t) - ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) - groupKey := models.GenerateGroupKey(orgID) - groupKey.NamespaceUID = folder.UID - gen := models.RuleGen.With(models.RuleGen.WithGroupKey(groupKey)) - - authorizedRule := gen.With(gen.WithUniqueGroupIndex()).Generate() - ruleStore.PutRule(context.Background(), &authorizedRule) - - unauthorizedRule := gen.With(gen.WithUniqueGroupIndex()).Generate() - ruleStore.PutRule(context.Background(), &unauthorizedRule) - - perms := createPermissionsForRules([]*models.AlertRule{&authorizedRule}, orgID) - req := createRequestContextWithPerms(orgID, perms, nil) - response := createService(ruleStore).RouteGetRuleByUID(req, authorizedRule.UID) - - require.Equal(t, http.StatusForbidden, response.Status()) - }) } func TestRouteGetRulesConfig(t *testing.T) { @@ -478,45 +461,6 @@ func TestRouteGetRulesConfig(t *testing.T) { func TestRouteGetRulesGroupConfig(t *testing.T) { gen := models.RuleGen - t.Run("fine-grained access is enabled", func(t *testing.T) { - t.Run("should check access to data source", func(t *testing.T) { - orgID := rand.Int63() - folder := randFolder() - ruleStore := fakes.NewRuleStore(t) - ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], folder) - groupKey := models.GenerateGroupKey(orgID) - groupKey.NamespaceUID = folder.UID - - expectedRules := gen.With(gen.WithGroupKey(groupKey)).GenerateManyRef(2, 6) - ruleStore.PutRule(context.Background(), expectedRules...) - - t.Run("and return Forbidden if user does not have access one of rules", func(t *testing.T) { - permissions := createPermissionsForRules(expectedRules[1:], orgID) - request := createRequestContextWithPerms(orgID, permissions, map[string]string{ - ":Namespace": folder.UID, - ":Groupname": groupKey.RuleGroup, - }) - response := createService(ruleStore).RouteGetRulesGroupConfig(request, folder.UID, groupKey.RuleGroup) - require.Equal(t, http.StatusForbidden, response.Status()) - }) - - t.Run("and return rules if user has access to all of them", func(t *testing.T) { - permissions := createPermissionsForRules(expectedRules, orgID) - request := createRequestContextWithPerms(orgID, permissions, map[string]string{ - ":Namespace": folder.UID, - ":Groupname": groupKey.RuleGroup, - }) - response := createService(ruleStore).RouteGetRulesGroupConfig(request, folder.UID, groupKey.RuleGroup) - - require.Equal(t, http.StatusAccepted, response.Status()) - result := &apimodels.RuleGroupConfigResponse{} - require.NoError(t, json.Unmarshal(response.Body(), result)) - require.NotNil(t, result) - require.Len(t, result.Rules, len(expectedRules)) - }) - }) - }) - t.Run("should return rules in group sorted by group index", func(t *testing.T) { orgID := rand.Int63() folder := randFolder() diff --git a/pkg/services/ngalert/api/testing.go b/pkg/services/ngalert/api/testing.go index 93434729e2f..fd55598cac3 100644 --- a/pkg/services/ngalert/api/testing.go +++ b/pkg/services/ngalert/api/testing.go @@ -153,3 +153,7 @@ func (f fakeRuleAccessControlService) AuthorizeRuleChanges(ctx context.Context, func (f fakeRuleAccessControlService) AuthorizeDatasourceAccessForRule(ctx context.Context, user identity.Requester, rule *models.AlertRule) error { return nil } + +func (f fakeRuleAccessControlService) AuthorizeDatasourceAccessForRuleGroup(ctx context.Context, user identity.Requester, rules models.RulesGroup) error { + return nil +}