From 5f80a29a289bf7251010496e1b22192d79185319 Mon Sep 17 00:00:00 2001 From: William Wernert Date: Fri, 12 Dec 2025 10:25:08 -0500 Subject: [PATCH] Alerting: Prevent users from saving rules to git-synced folders (#114944) --------- Co-authored-by: Yuri Tseretyan --- pkg/services/ngalert/api/api_ruler.go | 13 ++- pkg/services/ngalert/api/api_ruler_test.go | 61 +++++++++++ .../ngalert/api/prometheus/api_prometheus.go | 2 +- pkg/services/ngalert/eval/eval.go | 2 +- pkg/services/ngalert/models/alert_rule.go | 15 +++ .../ngalert/provisioning/alert_rules.go | 19 +++- .../ngalert/provisioning/alert_rules_test.go | 103 ++++++++++++++++++ pkg/services/ngalert/state/historian/loki.go | 2 +- 8 files changed, 209 insertions(+), 8 deletions(-) diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index d81f448f600..9fde7a2ec6f 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -412,11 +412,16 @@ func (srv RulerSrv) RoutePostNameRulesConfig(c *contextmodel.ReqContext, ruleGro deletePermanently = true } - namespace, err := srv.store.GetNamespaceByUID(c.Req.Context(), namespaceUID, c.GetOrgID(), c.SignedInUser) + f, err := srv.store.GetNamespaceByUID(c.Req.Context(), namespaceUID, c.GetOrgID(), c.SignedInUser) if err != nil { return toNamespaceErrorResponse(err) } + namespace := ngmodels.NewNamespace(f) + if err := namespace.ValidateForRuleStorage(); err != nil { + return ErrResp(http.StatusBadRequest, fmt.Errorf("%w: %s", ngmodels.ErrAlertRuleFailedValidation, err), "") + } + if err := srv.checkGroupLimits(ruleGroupConfig); err != nil { return ErrResp(http.StatusBadRequest, err, "") } @@ -841,10 +846,14 @@ func (srv RulerSrv) RouteUpdateNamespaceRules(c *contextmodel.ReqContext, body a return ErrResp(http.StatusBadRequest, errors.New("missing request body"), "") } - namespace, err := srv.store.GetNamespaceByUID(c.Req.Context(), namespaceUID, c.GetOrgID(), c.SignedInUser) + f, err := srv.store.GetNamespaceByUID(c.Req.Context(), namespaceUID, c.GetOrgID(), c.SignedInUser) if err != nil { return toNamespaceErrorResponse(err) } + namespace := ngmodels.NewNamespace(f) + if err := namespace.ValidateForRuleStorage(); err != nil { + return ErrResp(http.StatusBadRequest, fmt.Errorf("%w: %s", ngmodels.ErrAlertRuleFailedValidation, err), "") + } ruleGroups, _, err := srv.searchAuthorizedAlertRules(c.Req.Context(), authorizedRuleGroupQuery{ User: c.SignedInUser, diff --git a/pkg/services/ngalert/api/api_ruler_test.go b/pkg/services/ngalert/api/api_ruler_test.go index c6ddeee2a6c..e09088e4669 100644 --- a/pkg/services/ngalert/api/api_ruler_test.go +++ b/pkg/services/ngalert/api/api_ruler_test.go @@ -18,6 +18,7 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/infra/log" ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" @@ -1288,4 +1289,64 @@ func TestRouteUpdateNamespaceRules(t *testing.T) { updatedRules := getRecordedUpdatedRules(ruleStore) require.Empty(t, updatedRules) }) + + t.Run("should reject update when folder is managed by ManagerKindRepo", func(t *testing.T) { + ruleStore := fakes.NewRuleStore(t) + provisioningStore := fakes.NewFakeProvisioningStore() + + // Create a managed folder + managedFolder := randFolder() + managedFolder.ManagedBy = utils.ManagerKindRepo + ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], managedFolder) + + // Create some rules in the managed folder + ruleGen := models.RuleGen.With( + models.RuleGen.WithOrgID(orgID), + models.RuleGen.WithNamespaceUID(managedFolder.UID), + ) + rules := ruleGen.GenerateManyRef(2) + ruleStore.PutRule(context.Background(), rules...) + + permissions := createPermissionsForRules(rules, orgID) + requestCtx := createRequestContextWithPerms(orgID, permissions, nil) + + svc := createServiceWithProvenanceStore(ruleStore, provisioningStore) + response := svc.RouteUpdateNamespaceRules(requestCtx, apimodels.UpdateNamespaceRulesRequest{ + IsPaused: util.Pointer(true), + }, managedFolder.UID) + + require.Equal(t, http.StatusBadRequest, response.Status()) + require.Contains(t, string(response.Body()), "cannot store rules in folder managed by Git Sync") + + // Verify no rules were updated + updatedRules := getRecordedUpdatedRules(ruleStore) + require.Empty(t, updatedRules) + }) +} + +func TestRoutePostNameRulesConfig(t *testing.T) { + t.Run("should reject creation when folder is managed by ManagerKindRepo", func(t *testing.T) { + orgID := rand.Int63() + ruleStore := fakes.NewRuleStore(t) + + // Create a managed folder + managedFolder := randFolder() + managedFolder.ManagedBy = utils.ManagerKindRepo + ruleStore.Folders[orgID] = append(ruleStore.Folders[orgID], managedFolder) + + permissions := map[int64]map[string][]string{ + orgID: { + dashboards.ScopeFoldersProvider.GetResourceScopeUID(managedFolder.UID): {dashboards.ActionFoldersRead}, + }, + } + requestCtx := createRequestContextWithPerms(orgID, permissions, nil) + + svc := createService(ruleStore, nil) + response := svc.RoutePostNameRulesConfig(requestCtx, apimodels.PostableRuleGroupConfig{ + Name: "test-group", + }, managedFolder.UID) + + require.Equal(t, http.StatusBadRequest, response.Status()) + require.Contains(t, string(response.Body()), "cannot store rules in folder managed by Git Sync") + }) } diff --git a/pkg/services/ngalert/api/prometheus/api_prometheus.go b/pkg/services/ngalert/api/prometheus/api_prometheus.go index 412a0795469..36457cf7a30 100644 --- a/pkg/services/ngalert/api/prometheus/api_prometheus.go +++ b/pkg/services/ngalert/api/prometheus/api_prometheus.go @@ -296,7 +296,7 @@ func (srv PrometheusSrv) RouteGetRuleStatuses(c *contextmodel.ReqContext) respon allowedNamespaces := map[string]string{} for namespaceUID, folder := range namespaceMap { // only add namespaces that the user has access to rules in - hasAccess, err := srv.authz.HasAccessInFolder(c.Req.Context(), c.SignedInUser, ngmodels.Namespace(*folder.ToFolderReference())) + hasAccess, err := srv.authz.HasAccessInFolder(c.Req.Context(), c.SignedInUser, ngmodels.NewNamespace(folder)) if err != nil { ruleResponse.Status = "error" ruleResponse.Error = fmt.Sprintf("failed to get namespaces visible to the user: %s", err.Error()) diff --git a/pkg/services/ngalert/eval/eval.go b/pkg/services/ngalert/eval/eval.go index 687e9b2dd03..40e2a257e60 100644 --- a/pkg/services/ngalert/eval/eval.go +++ b/pkg/services/ngalert/eval/eval.go @@ -204,7 +204,7 @@ func IsNonRetryableError(err error) bool { return false } -// HasErrors returns true when Results contains at least one element and all elements are errors +// IsError returns true when Results contains at least one element and all elements are errors func (evalResults Results) IsError() bool { for _, r := range evalResults { if r.State != Error { diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 7ee223e8165..bc21e31cb45 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -24,6 +24,7 @@ import ( alertingModels "github.com/grafana/alerting/models" + "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/setting" @@ -397,6 +398,20 @@ type Namespaced interface { type Namespace folder.FolderReference +func NewNamespace(f *folder.Folder) Namespace { + return Namespace(*f.ToFolderReference()) +} + +func (n Namespace) ValidateForRuleStorage() error { + if n.UID == "" { + return fmt.Errorf("cannot store rules in folder without UID") + } + if n.ManagedBy == utils.ManagerKindRepo { + return fmt.Errorf("cannot store rules in folder managed by Git Sync") + } + return nil +} + func (n Namespace) GetNamespaceUID() string { return n.UID } diff --git a/pkg/services/ngalert/provisioning/alert_rules.go b/pkg/services/ngalert/provisioning/alert_rules.go index aa6c4c55b34..b5ff7506650 100644 --- a/pkg/services/ngalert/provisioning/alert_rules.go +++ b/pkg/services/ngalert/provisioning/alert_rules.go @@ -114,7 +114,7 @@ func (service *AlertRuleService) ListAlertRules(ctx context.Context, user identi } folderUIDs := make([]string, 0, len(folders)) for _, f := range folders { - access, err := service.authz.HasAccessInFolder(ctx, user, models.Namespace(*f.ToFolderReference())) + access, err := service.authz.HasAccessInFolder(ctx, user, models.NewNamespace(f)) if err != nil { return nil, nil, "", err } @@ -407,6 +407,9 @@ func (service *AlertRuleService) UpdateRuleGroup(ctx context.Context, user ident if err := models.ValidateRuleGroupInterval(intervalSeconds, service.baseIntervalSeconds); err != nil { return err } + if err := service.ensureNamespace(ctx, user, user.GetOrgID(), namespaceUID); err != nil { + return err + } return service.xact.InTransaction(ctx, func(ctx context.Context) error { query := &models.ListAlertRulesQuery{ OrgID: user.GetOrgID(), @@ -471,6 +474,10 @@ func (service *AlertRuleService) ReplaceRuleGroup(ctx context.Context, user iden return err } + if err := service.ensureNamespace(ctx, user, user.GetOrgID(), group.FolderUID); err != nil { + return err + } + // If the rule group is reserved for no-group rules, we cannot have multiple rules in it. if models.IsNoGroupRuleGroup(group.Title) && len(group.Rules) > 1 { return fmt.Errorf("rule group %s is reserved for no-group rules and cannot be used for rule groups with multiple rules", group.Title) @@ -1025,6 +1032,7 @@ func (service *AlertRuleService) checkGroupLimits(group models.AlertRuleGroup) e // ensureNamespace ensures that the rule has a valid namespace UID. // If the rule does not have a namespace UID or the namespace (folder) does not exist it will return an error. +// If the folder is managed by a manager, it will also return an error. func (service *AlertRuleService) ensureNamespace(ctx context.Context, user identity.Requester, orgID int64, namespaceUID string) error { if namespaceUID == "" { return fmt.Errorf("%w: folderUID must be set", models.ErrAlertRuleFailedValidation) @@ -1037,18 +1045,23 @@ func (service *AlertRuleService) ensureNamespace(ctx context.Context, user ident } // ensure the namespace exists - _, err := service.folderService.Get(ctx, &folder.GetFolderQuery{ + f, err := service.folderService.Get(ctx, &folder.GetFolderQuery{ OrgID: orgID, UID: &namespaceUID, SignedInUser: user, }) - if err != nil { + if err != nil || f == nil { if errors.Is(err, dashboards.ErrFolderNotFound) { return fmt.Errorf("%w: folder does not exist", models.ErrAlertRuleFailedValidation) } return err } + // check if the folder is managed by a manager + if err := models.NewNamespace(f).ValidateForRuleStorage(); err != nil { + return fmt.Errorf("%w: %s", models.ErrAlertRuleFailedValidation, err) + } + return nil } diff --git a/pkg/services/ngalert/provisioning/alert_rules_test.go b/pkg/services/ngalert/provisioning/alert_rules_test.go index c552f797429..ed001a527c4 100644 --- a/pkg/services/ngalert/provisioning/alert_rules_test.go +++ b/pkg/services/ngalert/provisioning/alert_rules_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/expr" "github.com/grafana/grafana/pkg/infra/db" @@ -867,6 +868,27 @@ func TestIntegrationAlertRuleService(t *testing.T) { require.NoError(t, err) require.Equal(t, int64(120), rule.IntervalSeconds) }) + + t.Run("UpdateRuleGroup should reject when folder is managed by a manager", func(t *testing.T) { + service, _, _, ac := initService(t) + ac.CanWriteAllRulesFunc = func(ctx context.Context, user identity.Requester) (bool, error) { + return true, nil + } + + managedFolderUID := "managed-folder-update-group" + fs := foldertest.NewFakeService() + fs.AddFolder(&folder.Folder{ + OrgID: orgID, + UID: managedFolderUID, + Title: "Managed Folder", + ManagedBy: utils.ManagerKindRepo, + }) + service.folderService = fs + + err := service.UpdateRuleGroup(context.Background(), u, managedFolderUID, "some-group", 120) + require.ErrorIs(t, err, models.ErrAlertRuleFailedValidation) + require.ErrorContains(t, err, "cannot store rules in folder managed by Git Sync") + }) } func TestIntegrationCreateAlertRule(t *testing.T) { @@ -1166,6 +1188,30 @@ func TestIntegrationCreateAlertRule(t *testing.T) { require.NoError(t, err) require.True(t, models.IsNoGroupRuleGroup(retrievedRule.RuleGroup), "Rule should be considered NoGroup rule") }) + + t.Run("should reject creation when folder is managed by a manager", func(t *testing.T) { + service, _, _, ac := initService(t) + ac.CanWriteAllRulesFunc = func(ctx context.Context, user identity.Requester) (bool, error) { + return true, nil + } + + managedFolderUID := "managed-folder" + fs := foldertest.NewFakeService() + fs.AddFolder(&folder.Folder{ + OrgID: orgID, + UID: managedFolderUID, + Title: "Managed Folder", + ManagedBy: utils.ManagerKindRepo, + }) + service.folderService = fs + + rule := dummyRule("test-managed-folder", orgID) + rule.NamespaceUID = managedFolderUID + + _, err := service.CreateAlertRule(context.Background(), u, rule, models.ProvenanceNone) + require.ErrorIs(t, err, models.ErrAlertRuleFailedValidation) + require.ErrorContains(t, err, "cannot store rules in folder managed by Git Sync") + }) } func TestUpdateAlertRule(t *testing.T) { @@ -1316,6 +1362,36 @@ func TestUpdateAlertRule(t *testing.T) { require.Equal(t, "nogroup-update-new", updated.Title) require.Equal(t, originalInterval, updated.IntervalSeconds) }) + + t.Run("should reject update when folder is managed by a manager", func(t *testing.T) { + service, ruleStore, provenanceStore, ac := initService(t) + ac.CanWriteAllRulesFunc = func(ctx context.Context, user identity.Requester) (bool, error) { + return true, nil + } + + managedFolderUID := "managed-folder-update" + fs := foldertest.NewFakeService() + fs.AddFolder(&folder.Folder{ + OrgID: orgID, + UID: managedFolderUID, + Title: "Managed Folder", + ManagedBy: utils.ManagerKindRepo, + }) + service.folderService = fs + + // Create an existing rule + existingRule := dummyRule("test-managed-folder-update", orgID) + existingRule.NamespaceUID = managedFolderUID + _, err := ruleStore.InsertAlertRules(context.Background(), models.NewUserUID(u), []models.InsertRule{{AlertRule: existingRule}}) + require.NoError(t, err) + require.NoError(t, provenanceStore.SetProvenance(context.Background(), &existingRule, orgID, models.ProvenanceNone)) + + // Try to update the rule + existingRule.Title = "Updated Title" + _, err = service.UpdateAlertRule(context.Background(), u, existingRule, models.ProvenanceNone) + require.ErrorIs(t, err, models.ErrAlertRuleFailedValidation) + require.ErrorContains(t, err, "cannot store rules in folder managed by Git Sync") + }) } func TestDeleteAlertRule(t *testing.T) { @@ -2054,6 +2130,33 @@ func TestReplaceGroup(t *testing.T) { require.Error(t, err) require.ErrorContains(t, err, "cannot move rule out of this group") }) + + t.Run("should reject replace when folder is managed by a manager", func(t *testing.T) { + service, _, _, ac := initService(t) + ac.CanWriteAllRulesFunc = func(ctx context.Context, user identity.Requester) (bool, error) { + return true, nil + } + + managedFolderUID := "managed-folder-replace" + fs := foldertest.NewFakeService() + fs.AddFolder(&folder.Folder{ + OrgID: orgID, + UID: managedFolderUID, + Title: "Managed Folder", + ManagedBy: utils.ManagerKindRepo, + }) + service.folderService = fs + + group := models.AlertRuleGroup{ + Title: "test-group", + FolderUID: managedFolderUID, + Interval: 60, + } + + err := service.ReplaceRuleGroup(context.Background(), u, group, models.ProvenanceNone, "") + require.ErrorIs(t, err, models.ErrAlertRuleFailedValidation) + require.ErrorContains(t, err, "cannot store rules in folder managed by Git Sync") + }) } func TestDeleteRuleGroup(t *testing.T) { diff --git a/pkg/services/ngalert/state/historian/loki.go b/pkg/services/ngalert/state/historian/loki.go index 137c18f031c..e07461f27bf 100644 --- a/pkg/services/ngalert/state/historian/loki.go +++ b/pkg/services/ngalert/state/historian/loki.go @@ -530,7 +530,7 @@ func (h *RemoteLokiBackend) getFolderUIDsForFilter(ctx context.Context, query mo uids := make([]string, 0, len(folders)) // now keep only UIDs of folder in which user can read rules. for _, f := range folders { - hasAccess, err := h.ac.HasAccessInFolder(ctx, query.SignedInUser, models.Namespace(*f.ToFolderReference())) + hasAccess, err := h.ac.HasAccessInFolder(ctx, query.SignedInUser, models.NewNamespace(f)) if err != nil { return nil, err }