From 032299011a26483567cce9b31c149dcd295e23d2 Mon Sep 17 00:00:00 2001 From: Moustafa Baiou Date: Thu, 10 Apr 2025 19:58:37 -0400 Subject: [PATCH] Alerting: Relax permissions for access a rule (#103664) This makes it so that it is: - No longer required to have datasource permissions to delete a rule. - No longer required to have datasource permissions to update non-query related fields of a rule. --- pkg/services/ngalert/accesscontrol/rules.go | 17 +- .../ngalert/accesscontrol/rules_test.go | 187 ++++++++++++++++-- pkg/services/ngalert/api/api_ruler.go | 18 -- pkg/services/ngalert/api/api_ruler_test.go | 40 +++- pkg/services/ngalert/store/deltas.go | 18 ++ pkg/services/ngalert/store/deltas_test.go | 76 +++++++ 6 files changed, 304 insertions(+), 52 deletions(-) diff --git a/pkg/services/ngalert/accesscontrol/rules.go b/pkg/services/ngalert/accesscontrol/rules.go index 8aa64c479e9..fdbd243c62a 100644 --- a/pkg/services/ngalert/accesscontrol/rules.go +++ b/pkg/services/ngalert/accesscontrol/rules.go @@ -180,13 +180,6 @@ func (r *RuleService) AuthorizeRuleChanges(ctx context.Context, user identity.Re }); err != nil { return err } - for _, rule := range change.Delete { - 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 // these are needed to check authorization for the rule create\update only once @@ -217,10 +210,12 @@ func (r *RuleService) AuthorizeRuleChanges(ctx context.Context, user identity.Re } for _, rule := range change.Update { - 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 + if rule.AffectsQuery() { + if err := r.HasAccessOrError(ctx, user, r.getRulesQueryEvaluator(rule.New), func() string { + return fmt.Sprintf("update alert rule query '%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. diff --git a/pkg/services/ngalert/accesscontrol/rules_test.go b/pkg/services/ngalert/accesscontrol/rules_test.go index eb15ffac8bb..a9006f5ef0f 100644 --- a/pkg/services/ngalert/accesscontrol/rules_test.go +++ b/pkg/services/ngalert/accesscontrol/rules_test.go @@ -2,6 +2,7 @@ package accesscontrol import ( "context" + "fmt" "math" "math/rand" "testing" @@ -23,6 +24,7 @@ import ( "github.com/grafana/grafana/pkg/services/ngalert/store" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/util" + "github.com/grafana/grafana/pkg/util/cmputil" ) func createAllCombinationsOfPermissions(permissions map[string][]string) []map[string][]string { @@ -108,6 +110,30 @@ func createUserWithPermissions(permissions map[string][]string) identity.Request }} } +func getShallowQueryDiffs(queries []models.AlertQuery) []cmputil.Diff { + result := make([]cmputil.Diff, 0, len(queries)) + for i := range queries { + result = append(result, []cmputil.Diff{ + { + Path: fmt.Sprintf("Data[%d].DatasourceUID", i), + }, + { + Path: fmt.Sprintf("Data[%d].Model", i), + }, + { + Path: fmt.Sprintf("Data[%d].RelativeTimeRange", i), + }, + { + Path: fmt.Sprintf("Data[%d].RefID", i), + }, + { + Path: fmt.Sprintf("Data[%d].QueryType", i), + }, + }...) + } + return result +} + func TestAuthorizeRuleChanges(t *testing.T) { groupKey := models.GenerateGroupKey(rand.Int63()) namespaceIdScope := dashboards.ScopeFoldersProvider.GetResourceScopeUID(groupKey.NamespaceUID) @@ -146,7 +172,7 @@ func TestAuthorizeRuleChanges(t *testing.T) { }, }, { - name: "if there are rules to delete it should check delete action and query for datasource", + name: "if there are rules to delete it should check delete action and NOT query for datasource", changes: func() *store.GroupDelta { rules := genWithGroupKey.GenerateManyRef(1, 5) rules2 := genWithGroupKey.GenerateManyRef(1, 5) @@ -171,12 +197,11 @@ func TestAuthorizeRuleChanges(t *testing.T) { ruleDelete: { namespaceIdScope, }, - datasources.ActionQuery: getDatasourceScopesForRules(c.Delete), } }, }, { - name: "if there are rules to update within the same namespace it should check update action and access to datasource", + name: "if there are rules with query updates within the same namespace it should check update action and access to datasource", changes: func() *store.GroupDelta { rules1 := genWithGroupKey.GenerateManyRef(1, 5) rules := genWithGroupKey.GenerateManyRef(1, 5) @@ -188,7 +213,7 @@ func TestAuthorizeRuleChanges(t *testing.T) { updates = append(updates, store.RuleDelta{ Existing: rule, New: cp, - Diff: nil, + Diff: getShallowQueryDiffs(cp.Data), }) } @@ -220,6 +245,55 @@ func TestAuthorizeRuleChanges(t *testing.T) { } }, }, + { + name: "if there are rules w/o query updates to update within the same namespace it should check update action", + changes: func() *store.GroupDelta { + rules1 := genWithGroupKey.GenerateManyRef(1, 5) + rules := genWithGroupKey.GenerateManyRef(1, 5) + updates := make([]store.RuleDelta, 0, len(rules)) + + for _, rule := range rules { + cp := models.CopyRule(rule) + cp.IsPaused = !rule.IsPaused + cp.Title = rule.Title + " updated" + updates = append(updates, store.RuleDelta{ + Existing: rule, + New: cp, + Diff: []cmputil.Diff{ + { + Path: "IsPaused", + }, + { + Path: "Title", + }, + }, + }) + } + + return &store.GroupDelta{ + GroupKey: groupKey, + AffectedGroups: map[models.AlertRuleGroupKey]models.RulesGroup{ + groupKey: append(rules, rules1...), + }, + New: nil, + Update: updates, + Delete: nil, + } + }, + permissions: func(c *store.GroupDelta) map[string][]string { + return map[string][]string{ + ruleRead: { + namespaceIdScope, + }, + dashboards.ActionFoldersRead: { + namespaceIdScope, + }, + ruleUpdate: { + namespaceIdScope, + }, + } + }, + }, { name: "if there are rules that are moved between namespaces it should check delete+add action and access to group where rules come from", changes: func() *store.GroupDelta { @@ -230,11 +304,22 @@ func TestAuthorizeRuleChanges(t *testing.T) { updates := make([]store.RuleDelta, 0, len(rules)) for _, rule := range rules { - cp := models.CopyRule(rule, gen.WithGroupKey(targetGroupKey), gen.WithQuery(gen.GenerateQuery())) + cp := models.CopyRule(rule, gen.WithGroupKey(targetGroupKey)) updates = append(updates, store.RuleDelta{ Existing: rule, New: cp, + Diff: []cmputil.Diff{ + { + Path: "OrgID", + }, + { + Path: "NamespaceUID", + }, + { + Path: "RuleGroup", + }, + }, }) } @@ -249,12 +334,6 @@ func TestAuthorizeRuleChanges(t *testing.T) { } }, permissions: func(c *store.GroupDelta) map[string][]string { - dsScopes := getDatasourceScopesForRules( - mapUpdates(c.Update, func(update store.RuleDelta) *models.AlertRule { - return update.New - }), - ) - var deleteScopes []string for key := range c.AffectedGroups { deleteScopes = append(deleteScopes, dashboards.ScopeFoldersProvider.GetResourceScopeUID(key.NamespaceUID)) @@ -265,7 +344,6 @@ func TestAuthorizeRuleChanges(t *testing.T) { ruleCreate: { dashboards.ScopeFoldersProvider.GetResourceScopeUID(c.GroupKey.NamespaceUID), }, - datasources.ActionQuery: dsScopes, } }, }, @@ -280,6 +358,68 @@ func TestAuthorizeRuleChanges(t *testing.T) { sourceGroup := genWithGroupKey.GenerateManyRef(1, 5) targetGroup := gen.With(gen.WithGroupKey(targetGroupKey)).GenerateManyRef(1, 5) + updates := make([]store.RuleDelta, 0, len(sourceGroup)) + toCopy := len(sourceGroup) + if toCopy > 1 { + toCopy = rand.Intn(toCopy-1) + 1 + } + for i := 0; i < toCopy; i++ { + rule := sourceGroup[0] + cp := models.CopyRule(rule, gen.WithGroupKey(targetGroupKey)) + + updates = append(updates, store.RuleDelta{ + Existing: rule, + New: cp, + Diff: []cmputil.Diff{ + { + Path: "OrgID", + }, + { + Path: "NamespaceUID", + }, + { + Path: "RuleGroup", + }, + }, + }) + } + + return &store.GroupDelta{ + GroupKey: targetGroupKey, + AffectedGroups: map[models.AlertRuleGroupKey]models.RulesGroup{ + groupKey: sourceGroup, + targetGroupKey: targetGroup, + }, + New: nil, + Update: updates, + Delete: nil, + } + }, + permissions: func(c *store.GroupDelta) map[string][]string { + return map[string][]string{ + ruleRead: { + dashboards.ScopeFoldersProvider.GetResourceScopeUID(c.GroupKey.NamespaceUID), + }, + dashboards.ActionFoldersRead: { + dashboards.ScopeFoldersProvider.GetResourceScopeUID(c.GroupKey.NamespaceUID), + }, + ruleUpdate: { + dashboards.ScopeFoldersProvider.GetResourceScopeUID(c.GroupKey.NamespaceUID), + }, + } + }, + }, + { + name: "if there are rules that are moved between groups in the same namespace AND the query is changed it should check update action and access to all groups (source+target) and datasources", + changes: func() *store.GroupDelta { + targetGroupKey := models.AlertRuleGroupKey{ + OrgID: groupKey.OrgID, + NamespaceUID: groupKey.NamespaceUID, + RuleGroup: util.GenerateShortUID(), + } + sourceGroup := genWithGroupKey.GenerateManyRef(1, 5) + targetGroup := gen.With(gen.WithGroupKey(targetGroupKey)).GenerateManyRef(1, 5) + updates := make([]store.RuleDelta, 0, len(sourceGroup)) toCopy := len(sourceGroup) if toCopy > 1 { @@ -292,6 +432,20 @@ func TestAuthorizeRuleChanges(t *testing.T) { updates = append(updates, store.RuleDelta{ Existing: rule, New: cp, + Diff: append( + []cmputil.Diff{ + { + Path: "OrgID", + }, + { + Path: "NamespaceUID", + }, + { + Path: "RuleGroup", + }, + }, + getShallowQueryDiffs(cp.Data)..., + ), }) } @@ -372,7 +526,11 @@ func TestAuthorizeRuleChanges(t *testing.T) { updates = append(updates, store.RuleDelta{ Existing: rule, New: cp, - Diff: nil, + Diff: []cmputil.Diff{ + { + Path: "NotificationSettings[0].Receiver", + }, + }, }) } @@ -397,9 +555,6 @@ func TestAuthorizeRuleChanges(t *testing.T) { ruleUpdate: { namespaceIdScope, }, - datasources.ActionQuery: getDatasourceScopesForRules(mapUpdates(c.Update, func(update store.RuleDelta) *models.AlertRule { - return update.New - })), accesscontrol.ActionAlertingReceiversRead: getReceiverScopesForRules(mapUpdates(c.Update, func(update store.RuleDelta) *models.AlertRule { return update.New })), diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index f7da9657a7e..7d3927a0f77 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -145,23 +145,12 @@ func (srv RulerSrv) RouteDeleteAlertRules(c *contextmodel.ReqContext, namespaceU } 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) @@ -177,17 +166,10 @@ 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 }) diff --git a/pkg/services/ngalert/api/api_ruler_test.go b/pkg/services/ngalert/api/api_ruler_test.go index 4b3c87910ab..8fc7e7212da 100644 --- a/pkg/services/ngalert/api/api_ruler_test.go +++ b/pkg/services/ngalert/api/api_ruler_test.go @@ -81,6 +81,24 @@ func TestRouteDeleteAlertRules(t *testing.T) { t.Run("when fine-grained access is enabled", func(t *testing.T) { t.Run("and group argument is empty", func(t *testing.T) { + t.Run("allow deleting without access to datasource", func(t *testing.T) { + ruleStore := initFakeRuleStore(t) + provisioningStore := fakes.NewFakeProvisioningStore() + + folderGen := gen.With(gen.WithNamespace(folder.ToFolderReference())) + + authorizedRulesInFolder := folderGen.With(gen.WithGroupPrefix("authz-")).GenerateManyRef(1, 5) + + ruleStore.PutRule(context.Background(), authorizedRulesInFolder...) + + permissions := createPermissionsForRulesWithoutDS(authorizedRulesInFolder, orgID) + requestCtx := createRequestContextWithPerms(orgID, permissions, nil) + + response := createServiceWithProvenanceStore(ruleStore, provisioningStore).RouteDeleteAlertRules(requestCtx, folder.UID, "") + + require.Equalf(t, 202, response.Status(), "Expected 202 but got %d: %v", response.Status(), string(response.Body())) + assertRulesDeleted(t, authorizedRulesInFolder, ruleStore) + }) t.Run("return Forbidden if user is not authorized to access any group in the folder", func(t *testing.T) { ruleStore := initFakeRuleStore(t) ruleStore.PutRule(context.Background(), gen.With(gen.WithNamespace(folder.ToFolderReference())).GenerateManyRef(1, 5)...) @@ -108,8 +126,6 @@ func TestRouteDeleteAlertRules(t *testing.T) { ruleStore.PutRule(context.Background(), authorizedRulesInFolder...) ruleStore.PutRule(context.Background(), provisionedRulesInFolder...) - // more rules in the same namespace but user does not have access to them - ruleStore.PutRule(context.Background(), folderGen.With(gen.WithGroupPrefix("unauthz")).GenerateManyRef(1, 5)...) permissions := createPermissionsForRules(append(authorizedRulesInFolder, provisionedRulesInFolder...), orgID) requestCtx := createRequestContextWithPerms(orgID, permissions, nil) @@ -130,8 +146,6 @@ func TestRouteDeleteAlertRules(t *testing.T) { require.NoError(t, err) ruleStore.PutRule(context.Background(), provisionedRulesInFolder...) - // more rules in the same namespace but user does not have access to them - ruleStore.PutRule(context.Background(), folderGen.With(gen.WithSameGroup()).GenerateManyRef(1, 5)...) permissions := createPermissionsForRules(provisionedRulesInFolder, orgID) requestCtx := createRequestContextWithPerms(orgID, permissions, nil) @@ -159,10 +173,8 @@ func TestRouteDeleteAlertRules(t *testing.T) { authorizedRulesInGroup := groupGen.GenerateManyRef(1, 5) ruleStore.PutRule(context.Background(), authorizedRulesInGroup...) - // more rules in the same group but user is not authorized to access them - ruleStore.PutRule(context.Background(), groupGen.GenerateManyRef(1, 5)...) - permissions := createPermissionsForRules(authorizedRulesInGroup, orgID) + permissions := createPermissionsForRules([]*models.AlertRule{}, orgID) requestCtx := createRequestContextWithPerms(orgID, permissions, nil) response := createService(ruleStore, nil).RouteDeleteAlertRules(requestCtx, folder.UID, authorizedRulesInGroup[0].RuleGroup) @@ -1014,3 +1026,17 @@ func createPermissionsForRules(rules []*models.AlertRule, orgID int64) map[int64 } return map[int64]map[string][]string{orgID: permissions} } + +func createPermissionsForRulesWithoutDS(rules []*models.AlertRule, orgID int64) map[int64]map[string][]string { + ns := map[string]any{} + permissions := map[string][]string{} + for _, rule := range rules { + if _, ok := ns[rule.NamespaceUID]; !ok { + scope := dashboards.ScopeFoldersProvider.GetResourceScopeUID(rule.NamespaceUID) + permissions[dashboards.ActionFoldersRead] = append(permissions[dashboards.ActionFoldersRead], scope) + permissions[ac.ActionAlertingRuleRead] = append(permissions[ac.ActionAlertingRuleRead], scope) + ns[rule.NamespaceUID] = struct{}{} + } + } + return map[int64]map[string][]string{orgID: permissions} +} diff --git a/pkg/services/ngalert/store/deltas.go b/pkg/services/ngalert/store/deltas.go index d59980604f1..f4fe12c98f8 100644 --- a/pkg/services/ngalert/store/deltas.go +++ b/pkg/services/ngalert/store/deltas.go @@ -3,6 +3,7 @@ package store import ( "context" "fmt" + "strings" "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/util/cmputil" @@ -11,12 +12,29 @@ import ( // AlertRuleFieldsToIgnoreInDiff contains fields that are ignored when calculating the RuleDelta.Diff. var AlertRuleFieldsToIgnoreInDiff = [...]string{"ID", "Version", "Updated", "UpdatedBy"} +// AlertRuleFieldsWhichAffectQuery contains fields which affect the rule's query(s) +var AlertRuleFieldsWhichAffectQuery = [...]string{"Data", "IntervalSeconds"} + type RuleDelta struct { Existing *models.AlertRule New *models.AlertRule Diff cmputil.DiffReport } +func (d *RuleDelta) AffectsQuery() bool { + if len(d.Diff) == 0 { + return false + } + for _, path := range d.Diff.Paths() { + for _, field := range AlertRuleFieldsWhichAffectQuery { + if strings.HasPrefix(path, field) { + return true + } + } + } + return false +} + type GroupDelta struct { GroupKey models.AlertRuleGroupKey // AffectedGroups contains all rules of all groups that are affected by these changes. diff --git a/pkg/services/ngalert/store/deltas_test.go b/pkg/services/ngalert/store/deltas_test.go index dbfeb556ce4..153fb7cadde 100644 --- a/pkg/services/ngalert/store/deltas_test.go +++ b/pkg/services/ngalert/store/deltas_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "reflect" "testing" "time" @@ -15,6 +16,7 @@ import ( "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/tests/fakes" "github.com/grafana/grafana/pkg/util" + "github.com/grafana/grafana/pkg/util/cmputil" ) func TestCalculateChanges(t *testing.T) { @@ -732,6 +734,80 @@ func TestCalculateRuleCreate(t *testing.T) { }) } +func TestDeltaAffectsQuery(t *testing.T) { + t.Run("returns false when there are no diffs", func(t *testing.T) { + delta := RuleDelta{ + Diff: cmputil.DiffReport{}, + } + assert.False(t, delta.AffectsQuery()) + }) + t.Run("returns true when diff contains a field that affects query", func(t *testing.T) { + delta := RuleDelta{ + Diff: cmputil.DiffReport{ + { + Path: "Data", + Left: reflect.ValueOf("old value"), + Right: reflect.ValueOf("new value"), + }, + }, + } + assert.True(t, delta.AffectsQuery()) + }) + t.Run("returns false when diff contains only fields that do not affect query", func(t *testing.T) { + delta := RuleDelta{ + Diff: cmputil.DiffReport{ + { + Path: "Title", + Left: reflect.ValueOf("old title"), + Right: reflect.ValueOf("new title"), + }, + }, + } + assert.False(t, delta.AffectsQuery()) + }) + t.Run("returns true when diff contains multiple fields, including one that affects query", func(t *testing.T) { + delta := RuleDelta{ + Diff: cmputil.DiffReport{ + { + Path: "Title", + Left: reflect.ValueOf("old title"), + Right: reflect.ValueOf("new title"), + }, + { + Path: "IntervalSeconds", + Left: reflect.ValueOf(10), + Right: reflect.ValueOf(20), + }, + }, + } + assert.True(t, delta.AffectsQuery()) + }) + t.Run("handles nested paths in diff", func(t *testing.T) { + delta := RuleDelta{ + Diff: cmputil.DiffReport{ + { + Path: "Data[0].Query", + Left: reflect.ValueOf("old query"), + Right: reflect.ValueOf("new query"), + }, + }, + } + assert.True(t, delta.AffectsQuery()) + }) + t.Run("returns false for empty diff paths", func(t *testing.T) { + delta := RuleDelta{ + Diff: cmputil.DiffReport{ + { + Path: "", + Left: reflect.ValueOf("old value"), + Right: reflect.ValueOf("new value"), + }, + }, + } + assert.False(t, delta.AffectsQuery()) + }) +} + // simulateSubmitted resets some fields of the structure that are not populated by API model to model conversion func simulateSubmitted(rule *models.AlertRule) { rule.ID = 0