Alerting: Remove requirement for datasource query on rule read (#87349)

* Remove requirement for datasource query for rule read

* Address PR comments

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.

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
 			}

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
 	}

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

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

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