Alerting: rule group update API to ignore deletes of rules user is not authorized to access (#46905)

* verify that the user has access to all data sources used by the rule that needs to be deleted from the group
* if a user is not authorized to access the rule, the rule is removed from the list to delete

pkg/services/ngalert/api/api_ruler.go

@@ -262,41 +262,53 @@ func (srv RulerSrv) RoutePostNameRulesConfig(c *models.ReqContext, ruleGroupConf
 	return srv.updateAlertRulesInGroup(c, namespace, ruleGroupConfig.Name, rules)
 }
 
+// updateAlertRulesInGroup calculates changes (rules to add,update,delete), verifies that the user is authorized to do the calculated changes and updates database.
+// All operations are performed in a single transaction
 func (srv RulerSrv) updateAlertRulesInGroup(c *models.ReqContext, namespace *models.Folder, groupName string, rules []*ngmodels.AlertRule) response.Response {
-	var groupChanges *changes = nil
+	var authorizedChanges *changes
 	hasAccess := accesscontrol.HasAccess(srv.ac, c)
-
 	err := srv.xactManager.InTransaction(c.Req.Context(), func(tranCtx context.Context) error {
-		var err error
+		logger := srv.log.New("namespace_uid", namespace.Uid, "group", groupName, "org_id", c.OrgId, "user_id", c.UserId)
-		groupChanges, err = calculateChanges(tranCtx, srv.store, c.SignedInUser.OrgId, namespace, groupName, rules)
+
+		groupChanges, err := calculateChanges(tranCtx, srv.store, c.SignedInUser.OrgId, namespace, groupName, rules)
 		if err != nil {
 			return err
 		}
 
 		if groupChanges.isEmpty() {
-			srv.log.Info("no changes detected in the request. Do nothing")
+			authorizedChanges = groupChanges
+			logger.Info("no changes detected in the request. Do nothing")
 			return nil
 		}
 
-		err = authorizeRuleChanges(namespace, groupChanges, func(evaluator accesscontrol.Evaluator) bool {
+		authorizedChanges, err = authorizeRuleChanges(namespace, groupChanges, func(evaluator accesscontrol.Evaluator) bool {
 			return hasAccess(accesscontrol.ReqOrgAdminOrEditor, evaluator)
 		})
 		if err != nil {
 			return err
 		}
 
-		srv.log.Debug("updating database with the changes", "group", groupName, "namespace", namespace.Uid, "add", len(groupChanges.New), "update", len(groupChanges.New), "delete", len(groupChanges.Delete))
+		if authorizedChanges.isEmpty() {
+			logger.Info("no authorized changes detected in the request. Do nothing", "not_authorized_add", len(groupChanges.New), "not_authorized_update", len(groupChanges.Update), "not_authorized_delete", len(groupChanges.Delete))
+			return nil
+		}
 
-		if len(groupChanges.Update) > 0 || len(groupChanges.New) > 0 {
+		if len(groupChanges.Delete) > len(authorizedChanges.Delete) {
-			upsert := make([]store.UpsertRule, 0, len(groupChanges.Update)+len(groupChanges.New))
+			logger.Info("user is not authorized to delete one or many rules in the group. those rules will be skipped", "expected", len(groupChanges.Delete), "authorized", len(authorizedChanges.Delete))
-			for _, update := range groupChanges.Update {
+		}
-				srv.log.Debug("updating rule", "uid", update.New.UID, "diff", update.Diff.String())
+
+		logger.Debug("updating database with the authorized changes", "add", len(authorizedChanges.New), "update", len(authorizedChanges.New), "delete", len(authorizedChanges.Delete))
+
+		if len(authorizedChanges.Update) > 0 || len(authorizedChanges.New) > 0 {
+			upsert := make([]store.UpsertRule, 0, len(authorizedChanges.Update)+len(authorizedChanges.New))
+			for _, update := range authorizedChanges.Update {
+				logger.Debug("updating rule", "rule_uid", update.New.UID, "diff", update.Diff.String())
 				upsert = append(upsert, store.UpsertRule{
 					Existing: update.Existing,
 					New:      *update.New,
 				})
 			}
-			for _, rule := range groupChanges.New {
+			for _, rule := range authorizedChanges.New {
 				upsert = append(upsert, store.UpsertRule{
 					Existing: nil,
 					New:      *rule,
@@ -308,9 +320,9 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *models.ReqContext, namespace *mod
 			}
 		}
 
-		if len(groupChanges.Delete) > 0 {
+		if len(authorizedChanges.Delete) > 0 {
-			UIDs := make([]string, 0, len(groupChanges.Delete))
+			UIDs := make([]string, 0, len(authorizedChanges.Delete))
-			for _, rule := range groupChanges.Delete {
+			for _, rule := range authorizedChanges.Delete {
 				UIDs = append(UIDs, rule.UID)
 			}
 
@@ -319,7 +331,7 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *models.ReqContext, namespace *mod
 			}
 		}
 
-		if len(groupChanges.New) > 0 {
+		if len(authorizedChanges.New) > 0 {
 			limitReached, err := srv.QuotaService.CheckQuotaReached(tranCtx, "alert_rule", &quota.ScopeParameters{
 				OrgId:  c.OrgId,
 				UserId: c.UserId,
@@ -347,21 +359,21 @@ func (srv RulerSrv) updateAlertRulesInGroup(c *models.ReqContext, namespace *mod
 		return ErrResp(http.StatusInternalServerError, err, "failed to update rule group")
 	}
 
-	for _, rule := range groupChanges.Update {
+	for _, rule := range authorizedChanges.Update {
 		srv.scheduleService.UpdateAlertRule(ngmodels.AlertRuleKey{
 			OrgID: c.SignedInUser.OrgId,
 			UID:   rule.Existing.UID,
 		})
 	}
 
-	for _, rule := range groupChanges.Delete {
+	for _, rule := range authorizedChanges.Delete {
 		srv.scheduleService.DeleteAlertRule(ngmodels.AlertRuleKey{
 			OrgID: c.SignedInUser.OrgId,
 			UID:   rule.UID,
 		})
 	}
 
-	if groupChanges.isEmpty() {
+	if authorizedChanges.isEmpty() {
 		return response.JSON(http.StatusAccepted, util.DynMap{"message": "no changes detected in the rule group"})
 	}
 

pkg/services/ngalert/api/authorization.go

@@ -185,49 +185,67 @@ func authorizeDatasourceAccessForRule(rule *ngmodels.AlertRule, evaluator func(e
 	return true
 }
 
-// authorizeRuleChanges analyzes changes in the rule group, determines what actions the user is trying to perform and check whether those actions are authorized.
+// authorizeRuleChanges analyzes changes in the rule group, and checks whether the changes are authorized.
-// If the user is not authorized to perform the changes the function returns ErrAuthorization with a description of what action is not authorized. If the evaluator function returns an error, the function returns it.
+// NOTE: if there are rules for deletion, and the user does not have access to data sources that a rule uses, the rule is removed from the list.
-func authorizeRuleChanges(namespace *models.Folder, changes *changes, evaluator func(evaluator ac.Evaluator) bool) error {
+// If the user is not authorized to perform the changes the function returns ErrAuthorization with a description of what action is not authorized.
+// Return changes that the user is authorized to perform or ErrAuthorization
+func authorizeRuleChanges(namespace *models.Folder, change *changes, evaluator func(evaluator ac.Evaluator) bool) (*changes, error) {
+	var result = &changes{
+		New:    change.New,
+		Update: change.Update,
+		Delete: change.Delete,
+	}
+
 	namespaceScope := dashboards.ScopeFoldersProvider.GetResourceScope(strconv.FormatInt(namespace.Id, 10))
-	if len(changes.Delete) > 0 {
+	if len(change.Delete) > 0 {
-		allowed := evaluator(ac.EvalPermission(ac.ActionAlertingRuleDelete, namespaceScope))
+		var allowedToDelete []*ngmodels.AlertRule
-		if !allowed {
+		for _, rule := range change.Delete {
-			return fmt.Errorf("%w user cannot delete alert rules that belong to folder %s", ErrAuthorization, namespace.Title)
+			dsAllowed := authorizeDatasourceAccessForRule(rule, evaluator)
+			if dsAllowed {
+				allowedToDelete = append(allowedToDelete, rule)
+			}
 		}
+		if len(allowedToDelete) > 0 {
+			allowed := evaluator(ac.EvalPermission(ac.ActionAlertingRuleDelete, namespaceScope))
+			if !allowed {
+				return nil, fmt.Errorf("%w to delete alert rules that belong to folder %s", ErrAuthorization, namespace.Title)
+			}
+		}
+		result.Delete = allowedToDelete
 	}
 
 	var addAuthorized, updateAuthorized bool
 
-	if len(changes.New) > 0 {
+	if len(change.New) > 0 {
 		addAuthorized = evaluator(ac.EvalPermission(ac.ActionAlertingRuleCreate, namespaceScope))
 		if !addAuthorized {
-			return fmt.Errorf("%w user cannot create alert rules in the folder %s", ErrAuthorization, namespace.Title)
+			return nil, fmt.Errorf("%w to create alert rules in the folder %s", ErrAuthorization, namespace.Title)
 		}
-		for _, rule := range changes.New {
+		for _, rule := range change.New {
 			dsAllowed := authorizeDatasourceAccessForRule(rule, evaluator)
 			if !dsAllowed {
-				return fmt.Errorf("%w to create a new alert rule '%s' because the user does not have read permissions for one or many datasources the rule uses", ErrAuthorization, rule.Title)
+				return nil, fmt.Errorf("%w to create a new alert rule '%s' because the user does not have read permissions for one or many datasources the rule uses", ErrAuthorization, rule.Title)
 			}
 		}
 	}
 
-	for _, rule := range changes.Update {
+	for _, rule := range change.Update {
 		dsAllowed := authorizeDatasourceAccessForRule(rule.New, evaluator)
 		if !dsAllowed {
-			return fmt.Errorf("%w to update alert rule '%s' (UID: %s) because the user does not have read permissions for one or many datasources the rule uses", ErrAuthorization, rule.Existing.Title, rule.Existing.UID)
+			return nil, fmt.Errorf("%w to update alert rule '%s' (UID: %s) because the user does not have read permissions for one or many datasources the rule uses", ErrAuthorization, rule.Existing.Title, rule.Existing.UID)
 		}
 
 		// 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.
 		if rule.Existing.NamespaceUID != rule.New.NamespaceUID {
 			allowed := evaluator(ac.EvalAll(ac.EvalPermission(ac.ActionAlertingRuleDelete, dashboards.ScopeFoldersProvider.GetResourceScopeUID(rule.Existing.NamespaceUID))))
 			if !allowed {
-				return fmt.Errorf("%w to delete alert rules from folder UID %s", ErrAuthorization, rule.Existing.NamespaceUID)
+				return nil, fmt.Errorf("%w to delete alert rules from folder UID %s", ErrAuthorization, rule.Existing.NamespaceUID)
 			}
 
 			if !addAuthorized {
 				addAuthorized = evaluator(ac.EvalPermission(ac.ActionAlertingRuleCreate, namespaceScope))
 				if !addAuthorized {
-					return fmt.Errorf("%w to create alert rules in the folder '%s'", ErrAuthorization, namespace.Title)
+					return nil, fmt.Errorf("%w to create alert rules in the folder '%s'", ErrAuthorization, namespace.Title)
 				}
 			}
 			continue
@@ -236,9 +254,9 @@ func authorizeRuleChanges(namespace *models.Folder, changes *changes, evaluator
 		if !updateAuthorized { // if it is false then the authorization was not checked. If it is true then the user is authorized to update rules
 			updateAuthorized = evaluator(ac.EvalAll(ac.EvalPermission(ac.ActionAlertingRuleUpdate, namespaceScope)))
 			if !updateAuthorized {
-				return fmt.Errorf("%w to update alert rules that belong to folder %s", ErrAuthorization, namespace.Title)
+				return nil, fmt.Errorf("%w to update alert rules that belong to folder %s", ErrAuthorization, namespace.Title)
 			}
 		}
 	}
-	return nil
+	return result, nil
 }

pkg/services/ngalert/api/authorization_test.go

@@ -78,23 +78,6 @@ func TestAuthorizeRuleChanges(t *testing.T) {
 		changes     func() *changes
 		permissions func(c *changes) map[string][]string
 	}{
-		{
-			name: "if there are rules to delete it should check delete action and access to data sources",
-			changes: func() *changes {
-				return &changes{
-					New:    nil,
-					Update: nil,
-					Delete: models.GenerateAlertRules(rand.Intn(4)+1, models.AlertRuleGen(withNamespace(namespace))),
-				}
-			},
-			permissions: func(c *changes) map[string][]string {
-				return map[string][]string{
-					ac.ActionAlertingRuleDelete: {
-						namespaceIdScope,
-					},
-				}
-			},
-		},
 		{
 			name: "if there are rules to add it should check create action and query for datasource",
 			changes: func() *changes {
@@ -203,19 +186,20 @@ func TestAuthorizeRuleChanges(t *testing.T) {
 
 			groupChanges := testCase.changes()
 
-			err := authorizeRuleChanges(namespace, groupChanges, func(evaluator ac.Evaluator) bool {
+			result, err := authorizeRuleChanges(namespace, groupChanges, func(evaluator ac.Evaluator) bool {
 				response, err := evaluator.Evaluate(make(map[string][]string))
 				require.False(t, response)
 				require.NoError(t, err)
 				executed = true
 				return false
 			})
+			require.Nil(t, result)
 			require.Error(t, err)
 			require.Truef(t, executed, "evaluation function is expected to be called but it was not.")
 
 			permissions := testCase.permissions(groupChanges)
 			executed = false
-			err = authorizeRuleChanges(namespace, groupChanges, func(evaluator ac.Evaluator) bool {
+			result, err = authorizeRuleChanges(namespace, groupChanges, func(evaluator ac.Evaluator) bool {
 				response, err := evaluator.Evaluate(permissions)
 				require.Truef(t, response, "provided permissions [%v] is not enough for requested permissions [%s]", testCase.permissions, evaluator.GoString())
 				require.NoError(t, err)
@@ -223,11 +207,161 @@ func TestAuthorizeRuleChanges(t *testing.T) {
 				return true
 			})
 			require.NoError(t, err)
+			require.Equal(t, groupChanges, result)
 			require.Truef(t, executed, "evaluation function is expected to be called but it was not.")
 		})
 	}
 }
 
+func TestAuthorizeRuleDelete(t *testing.T) {
+	namespace := randFolder()
+	namespaceIdScope := dashboards.ScopeFoldersProvider.GetResourceScope(strconv.FormatInt(namespace.Id, 10))
+
+	getScopes := func(rules []*models.AlertRule) []string {
+		var scopes []string
+		for _, rule := range rules {
+			for _, query := range rule.Data {
+				scopes = append(scopes, dashboards.ScopeFoldersProvider.GetResourceScopeUID(query.DatasourceUID))
+			}
+		}
+		return scopes
+	}
+
+	testCases := []struct {
+		name        string
+		changes     func() *changes
+		permissions func(c *changes) map[string][]string
+		assert      func(t *testing.T, orig, authz *changes, err error)
+	}{
+		{
+			name: "should validate check access to data source and folder",
+			changes: func() *changes {
+				return &changes{
+					New:    nil,
+					Update: nil,
+					Delete: models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withNamespace(namespace))),
+				}
+			},
+			permissions: func(c *changes) map[string][]string {
+				return map[string][]string{
+					ac.ActionAlertingRuleDelete: {
+						namespaceIdScope,
+					},
+					datasources.ActionQuery: getScopes(c.Delete),
+				}
+			},
+			assert: func(t *testing.T, orig, authz *changes, err error) {
+				require.NoError(t, err)
+				require.Equal(t, orig, authz)
+			},
+		},
+		{
+			name: "should remove rules user does not have access to data source",
+			changes: func() *changes {
+				return &changes{
+					New:    nil,
+					Update: nil,
+					Delete: models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withNamespace(namespace))),
+				}
+			},
+			permissions: func(c *changes) map[string][]string {
+				return map[string][]string{
+					ac.ActionAlertingRuleDelete: {
+						namespaceIdScope,
+					},
+					datasources.ActionQuery: {
+						getScopes(c.Delete[:1])[0],
+					},
+				}
+			},
+			assert: func(t *testing.T, orig, authz *changes, err error) {
+				require.NoError(t, err)
+				require.Greater(t, len(orig.Delete), len(authz.Delete))
+			},
+		},
+		{
+			name: "should not fail if no changes other than unauthorized",
+			changes: func() *changes {
+				return &changes{
+					New:    nil,
+					Update: nil,
+					Delete: models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withNamespace(namespace))),
+				}
+			},
+			permissions: func(c *changes) map[string][]string {
+				return map[string][]string{
+					ac.ActionAlertingRuleDelete: {
+						namespaceIdScope,
+					},
+				}
+			},
+			assert: func(t *testing.T, orig, authz *changes, err error) {
+				require.NoError(t, err)
+				require.False(t, orig.isEmpty())
+				require.True(t, authz.isEmpty())
+			},
+		},
+		{
+			name: "should not fail if there are changes and no rules can be deleted",
+			changes: func() *changes {
+				return &changes{
+					New:    models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withNamespace(namespace))),
+					Update: nil,
+					Delete: models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withNamespace(namespace))),
+				}
+			},
+			permissions: func(c *changes) map[string][]string {
+				return map[string][]string{
+					ac.ActionAlertingRuleDelete: {
+						namespaceIdScope,
+					},
+					ac.ActionAlertingRuleCreate: {
+						namespaceIdScope,
+					},
+					datasources.ActionQuery: getScopes(c.New),
+				}
+			},
+			assert: func(t *testing.T, _, c *changes, err error) {
+				require.NoError(t, err)
+				require.Empty(t, c.Delete)
+			},
+		},
+		{
+			name: "should fail if no access to folder",
+			changes: func() *changes {
+				return &changes{
+					New:    nil,
+					Update: nil,
+					Delete: models.GenerateAlertRules(rand.Intn(4)+2, models.AlertRuleGen(withNamespace(namespace))),
+				}
+			},
+			permissions: func(c *changes) map[string][]string {
+				return map[string][]string{
+					datasources.ActionQuery: getScopes(c.Delete),
+				}
+			},
+			assert: func(t *testing.T, _, c *changes, err error) {
+				require.ErrorIs(t, err, ErrAuthorization)
+				require.Nil(t, c)
+			},
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			groupChanges := testCase.changes()
+			permissions := testCase.permissions(groupChanges)
+			result, err := authorizeRuleChanges(namespace, groupChanges, func(evaluator ac.Evaluator) bool {
+				response, err := evaluator.Evaluate(permissions)
+				require.NoError(t, err)
+				return response
+			})
+
+			testCase.assert(t, groupChanges, result, err)
+		})
+	}
+}
+
 func TestCheckDatasourcePermissionsForRule(t *testing.T) {
 	rule := models.AlertRuleGen()()