public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[v10.1.x] Alerting: Fix deleting rules in a folder with matching UID in another organization (#79007)

Browse Source 
* Alerting: Fix deleting rules in a folder with matching UID in another organization (#78258)

* Remove usage of obsolete function for deleting alert rules under folder

* Apply suggestion from code review

* Update tests

(cherry picked from commit 6d4625ad52)

* fixup
This commit is contained in:
Sofia Papagiannaki
2023-12-05 13:30:17 +02:00
committed by GitHub
parent 7a0ee7044b
commit 5bed94f873
11 changed files with 80 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/api/apierrors/folder.go
+1 -2
View File
@@ -18,8 +18,7 @@ func ToFolderErrorResponse(err error) response.Response {
if errors.Is(err, dashboards.ErrFolderTitleEmpty) ||
errors.Is(err, dashboards.ErrDashboardTypeMismatch) ||
errors.Is(err, dashboards.ErrDashboardInvalidUid) ||
errors.Is(err, dashboards.ErrDashboardUidTooLong) ||
errors.Is(err, dashboards.ErrFolderContainsAlertRules) {
errors.Is(err, dashboards.ErrDashboardUidTooLong) {
return response.Error(400, err.Error(), nil)
}
pkg/services/dashboards/database/database.go
-31
View File
@@ -721,10 +721,6 @@ func (d *dashboardStore) deleteDashboard(cmd *dashboards.DeleteDashboardCommand,
if err != nil {
return err
}
if err := deleteFolderAlertRules(sess, dashboard, cmd.ForceDeleteFolderRules); err != nil {
return err
}
} else {
if err := d.deleteResourcePermissions(sess, dashboard.OrgID, ac.GetResourceScopeUID("dashboards", dashboard.UID)); err != nil {
return err
@@ -819,33 +815,6 @@ func (d *dashboardStore) deleteChildrenDashboardAssociations(sess *db.Session, d
return nil
}
func deleteFolderAlertRules(sess *db.Session, dashboard dashboards.Dashboard, forceDeleteFolderAlertRules bool) error {
var existingRuleID int64
exists, err := sess.Table("alert_rule").Where("namespace_uid = (SELECT uid FROM dashboard WHERE id = ?)", dashboard.ID).Cols("id").Get(&existingRuleID)
if err != nil {
return err
}
if exists {
if !forceDeleteFolderAlertRules {
return fmt.Errorf("folder cannot be deleted: %w", dashboards.ErrFolderContainsAlertRules)
}
// Delete all rules under this folder.
deleteNGAlertsByFolder := []string{
"DELETE FROM alert_rule WHERE namespace_uid = (SELECT uid FROM dashboard WHERE id = ?)",
"DELETE FROM alert_rule_version WHERE rule_namespace_uid = (SELECT uid FROM dashboard WHERE id = ?)",
}
for _, sql := range deleteNGAlertsByFolder {
_, err := sess.Exec(sql, dashboard.ID)
if err != nil {
return err
}
}
}
return nil
}
func createEntityEvent(dashboard *dashboards.Dashboard, eventType store.EntityEventType) *store.EntityEvent {
var entityEvent *store.EntityEvent
if dashboard.IsFolder {
pkg/services/dashboards/database/database_test.go
-40
View File
@@ -3,7 +3,6 @@ package database
import (
"context"
"encoding/json"
"errors"
"fmt"
"testing"
"time"
@@ -254,45 +253,6 @@ func TestIntegrationDashboardDataAccess(t *testing.T) {
require.NoError(t, err)
})
t.Run("Should be not able to delete a dashboard if force delete rules is disabled", func(t *testing.T) {
setup()
deleteCmd := &dashboards.DeleteDashboardCommand{ID: savedFolder.ID, ForceDeleteFolderRules: false}
err := dashboardStore.DeleteDashboard(context.Background(), deleteCmd)
require.True(t, errors.Is(err, dashboards.ErrFolderContainsAlertRules))
})
t.Run("Should be able to delete a dashboard folder and its children if force delete rules is enabled", func(t *testing.T) {
setup()
deleteCmd := &dashboards.DeleteDashboardCommand{ID: savedFolder.ID, ForceDeleteFolderRules: true}
err := dashboardStore.DeleteDashboard(context.Background(), deleteCmd)
require.NoError(t, err)
query := dashboards.FindPersistedDashboardsQuery{
OrgId: 1,
FolderIds: []int64{savedFolder.ID},
SignedInUser: &user.SignedInUser{},
}
res, err := dashboardStore.FindDashboards(context.Background(), &query)
require.NoError(t, err)
require.Equal(t, len(res), 0)
err = sqlStore.WithDbSession(context.Background(), func(sess *db.Session) error {
var existingRuleID int64
exists, err := sess.Table("alert_rule").Where("namespace_uid = (SELECT uid FROM dashboard WHERE id = ?)", savedFolder.ID).Cols("id").Get(&existingRuleID)
require.NoError(t, err)
require.False(t, exists)
var existingRuleVersionID int64
exists, err = sess.Table("alert_rule_version").Where("rule_namespace_uid = (SELECT uid FROM dashboard WHERE id = ?)", savedFolder.ID).Cols("id").Get(&existingRuleVersionID)
require.NoError(t, err)
require.False(t, exists)
return nil
})
require.NoError(t, err)
})
t.Run("Should return error if no dashboard is found for update when dashboard id is greater than zero", func(t *testing.T) {
cmd := dashboards.SaveDashboardCommand{
OrgID: 1,
pkg/services/dashboards/errors.go
+7 -8
View File
@@ -117,14 +117,13 @@ var (
Status: "not-found",
}
ErrFolderNotFound = errors.New("folder not found")
ErrFolderVersionMismatch = errors.New("the folder has been changed by someone else")
ErrFolderTitleEmpty = errors.New("folder title cannot be empty")
ErrFolderWithSameUIDExists = errors.New("a folder/dashboard with the same uid already exists")
ErrFolderInvalidUID = errors.New("invalid uid for folder provided")
ErrFolderSameNameExists = errors.New("a folder or dashboard in the general folder with the same name already exists")
ErrFolderAccessDenied = errors.New("access denied to folder")
ErrFolderContainsAlertRules = errors.New("folder contains alert rules")
ErrFolderNotFound = errors.New("folder not found")
ErrFolderVersionMismatch = errors.New("the folder has been changed by someone else")
ErrFolderTitleEmpty = errors.New("folder title cannot be empty")
ErrFolderWithSameUIDExists = errors.New("a folder/dashboard with the same uid already exists")
ErrFolderInvalidUID = errors.New("invalid uid for folder provided")
ErrFolderSameNameExists = errors.New("a folder or dashboard in the general folder with the same name already exists")
ErrFolderAccessDenied = errors.New("access denied to folder")
)
// DashboardErr represents a dashboard error.
pkg/services/dashboards/service/dashboard_service_test.go
+1 -1
View File
@@ -206,7 +206,7 @@ func TestDashboardService(t *testing.T) {
t.Run("Given non provisioned dashboard", func(t *testing.T) {
t.Run("DeleteProvisionedDashboard should delete the dashboard", func(t *testing.T) {
args := &dashboards.DeleteDashboardCommand{OrgID: 1, ID: 1, ForceDeleteFolderRules: false}
args := &dashboards.DeleteDashboardCommand{OrgID: 1, ID: 1}
fakeStore.On("DeleteDashboard", mock.Anything, args).Return(nil).Once()
err := service.DeleteProvisionedDashboard(context.Background(), 1, 1)
require.NoError(t, err)
pkg/services/folder/folderimpl/folder.go
+15 -2
View File
@@ -502,8 +502,8 @@ func (s *Service) Delete(ctx context.Context, cmd *folder.DeleteFolderCommand) e
result = append(result, subfolders...)
}
for _, folder := range result {
dashFolder, err := s.dashboardFolderStore.GetFolderByUID(ctx, cmd.OrgID, folder)
for _, f := range result {
dashFolder, err := s.dashboardFolderStore.GetFolderByUID(ctx, cmd.OrgID, f)
if err != nil {
return err
}
@@ -512,6 +512,19 @@ func (s *Service) Delete(ctx context.Context, cmd *folder.DeleteFolderCommand) e
if err := s.deleteChildrenInFolder(ctx, dashFolder.OrgID, dashFolder.UID, cmd.SignedInUser); err != nil {
return err
}
} else {
alertRuleSrv, ok := s.registry[entity.StandardKindAlertRule]
if !ok {
return folder.ErrInternal.Errorf("no alert rule service found in registry")
}
alertRulesInFolder, err := alertRuleSrv.CountInFolder(ctx, dashFolder.OrgID, dashFolder.UID, cmd.SignedInUser)
if err != nil {
s.log.Error("failed to count alert rules in folder", "error", err)
return err
}
if alertRulesInFolder > 0 {
return folder.ErrFolderNotEmpty.Errorf("folder contains %d alert rules", alertRulesInFolder)
}
}
err = s.legacyDelete(ctx, cmd, dashFolder)
pkg/services/folder/folderimpl/folder_test.go
+19 -4
View File
@@ -73,6 +73,16 @@ func TestIntegrationFolderService(t *testing.T) {
cfg := setting.NewCfg()
features := featuremgmt.WithFeatures()
ac := acmock.New().WithPermissions([]accesscontrol.Permission{
{Action: accesscontrol.ActionAlertingRuleDelete, Scope: dashboards.ScopeFoldersAll},
})
alertingStore := ngstore.DBstore{
SQLStore: db,
Cfg: cfg.UnifiedAlerting,
Logger: log.New("test-alerting-store"),
AccessControl: ac,
}
service := &Service{
cfg: cfg,
log: log.New("test-folder-service"),
@@ -83,8 +93,11 @@ func TestIntegrationFolderService(t *testing.T) {
bus: bus.ProvideBus(tracing.InitializeTracerForTest()),
db: db,
accessControl: acimpl.ProvideAccessControl(cfg),
registry: make(map[string]folder.RegistryService),
}
require.NoError(t, service.RegisterService(alertingStore))
t.Run("Given user has no permissions", func(t *testing.T) {
origNewGuardian := guardian.New
guardian.MockDashboardGuardian(&guardian.FakeDashboardGuardian{})
@@ -362,8 +375,10 @@ func TestIntegrationNestedFolderService(t *testing.T) {
signedInUser := user.SignedInUser{UserID: 1, OrgID: orgID, Permissions: map[int64]map[string][]string{
orgID: {
dashboards.ActionFoldersCreate: {},
dashboards.ActionFoldersWrite: {dashboards.ScopeFoldersAll}},
dashboards.ActionFoldersCreate: {},
dashboards.ActionFoldersWrite: {dashboards.ScopeFoldersAll},
accesscontrol.ActionAlertingRuleDelete: {dashboards.ScopeFoldersAll},
},
}}
createCmd := folder.CreateFolderCommand{
OrgID: orgID,
@@ -567,7 +582,7 @@ func TestIntegrationNestedFolderService(t *testing.T) {
prefix: "flagon-noforce",
depth: 3,
forceDelete: false,
deletionErr: dashboards.ErrFolderContainsAlertRules,
deletionErr: folder.ErrFolderNotEmpty,
desc: "With nested folder feature flag on and no force deletion of rules",
},
{
@@ -586,7 +601,7 @@ func TestIntegrationNestedFolderService(t *testing.T) {
prefix: "flagoff-noforce",
depth: 1,
forceDelete: false,
deletionErr: dashboards.ErrFolderContainsAlertRules,
deletionErr: folder.ErrFolderNotEmpty,
desc: "With nested folder feature flag off and no force deletion of rules",
},
}
pkg/services/folder/model.go
+1
View File
@@ -16,6 +16,7 @@ var ErrDatabaseError = errutil.NewBase(errutil.StatusInternal, "folder.database-
var ErrInternal = errutil.NewBase(errutil.StatusInternal, "folder.internal")
var ErrCircularReference = errutil.NewBase(errutil.StatusBadRequest, "folder.circular-reference", errutil.WithPublicMessage("Circular reference detected"))
var ErrTargetRegistrySrvConflict = errutil.NewBase(errutil.StatusInternal, "folder.target-registry-srv-conflict")
var ErrFolderNotEmpty = errutil.NewBase(errutil.StatusBadRequest, "folder.not-empty", errutil.WithPublicMessage("Folder cannot be deleted: folder is not empty"))
const (
GeneralFolderUID = "general"
pkg/services/ngalert/store/alert_rule.go
+12
View File
@@ -9,6 +9,7 @@ import (
"github.com/google/uuid"
"github.com/grafana/grafana/pkg/infra/db"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/folder"
@@ -587,6 +588,17 @@ func (st DBstore) GetAlertRulesForScheduling(ctx context.Context, query *ngmodel
// DeleteInFolder deletes the rules contained in a given folder along with their associated data.
func (st DBstore) DeleteInFolder(ctx context.Context, orgID int64, folderUID string, user *user.SignedInUser) error {
evaluator := accesscontrol.EvalPermission(accesscontrol.ActionAlertingRuleDelete, dashboards.ScopeFoldersProvider.GetResourceScopeName(folderUID))
canSave, err := st.AccessControl.Evaluate(ctx, user, evaluator)
if err != nil {
st.Logger.Error("Failed to evaluate access control", "error", err)
return err
}
if !canSave {
st.Logger.Error("user is not allowed to delete alert rules in folder", "folder", folderUID, "user")
return dashboards.ErrFolderAccessDenied
}
rules, err := st.ListAlertRules(ctx, &ngmodels.ListAlertRulesQuery{
OrgID: orgID,
NamespaceUIDs: []string{folderUID},
pkg/services/ngalert/store/alert_rule_test.go
+19 -5
View File
@@ -12,6 +12,8 @@ import (
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/infra/log/logtest"
"github.com/grafana/grafana/pkg/infra/tracing"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/folder"
"github.com/grafana/grafana/pkg/services/folder/folderimpl"
@@ -24,6 +26,7 @@ import (
"golang.org/x/exp/rand"
"github.com/grafana/grafana/pkg/infra/db"
acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock"
"github.com/grafana/grafana/pkg/services/ngalert/models"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/util"
@@ -474,12 +477,23 @@ func TestIntegration_DeleteInFolder(t *testing.T) {
}
rule := createRule(t, store, nil)
err := store.DeleteInFolder(context.Background(), rule.OrgID, rule.NamespaceUID, nil)
require.NoError(t, err)
t.Run("should not be able to delete folder without permissions to delete rules", func(t *testing.T) {
store.AccessControl = acmock.New()
err := store.DeleteInFolder(context.Background(), rule.OrgID, rule.NamespaceUID, &user.SignedInUser{})
require.ErrorIs(t, err, dashboards.ErrFolderAccessDenied)
})
c, err := store.CountInFolder(context.Background(), rule.OrgID, rule.NamespaceUID, nil)
require.NoError(t, err)
require.Equal(t, int64(0), c)
t.Run("should be able to delete folder with permissions to delete rules", func(t *testing.T) {
store.AccessControl = acmock.New().WithPermissions([]accesscontrol.Permission{
{Action: accesscontrol.ActionAlertingRuleDelete, Scope: dashboards.ScopeFoldersAll},
})
err := store.DeleteInFolder(context.Background(), rule.OrgID, rule.NamespaceUID, &user.SignedInUser{})
require.NoError(t, err)
c, err := store.CountInFolder(context.Background(), rule.OrgID, rule.NamespaceUID, &user.SignedInUser{})
require.NoError(t, err)
require.Equal(t, int64(0), c)
})
}
func createRule(t *testing.T, store *DBstore, generate func() *models.AlertRule) *models.AlertRule {
pkg/tests/api/alerting/api_alertmanager_test.go
+5 -1
View File
@@ -17,6 +17,7 @@ import (
"github.com/stretchr/testify/require"
"github.com/grafana/grafana/pkg/expr"
"github.com/grafana/grafana/pkg/util/errutil"
apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models"
@@ -799,7 +800,10 @@ func TestIntegrationDeleteFolderWithRules(t *testing.T) {
b, err := io.ReadAll(resp.Body)
require.NoError(t, err)
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
require.JSONEq(t, `{"message":"folder cannot be deleted: folder contains alert rules"}`, string(b))
var errutilErr errutil.PublicError
err = json.Unmarshal(b, &errutilErr)
require.NoError(t, err)
assert.Equal(t, "Folder cannot be deleted: folder is not empty", errutilErr.Message)
}
// Next, the editor can delete the folder if forceDeleteRules is true.