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

@@ -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.

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
 					})),

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
 	})

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}
+}

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.

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