From 5303a1cc7eda648f65b37d677a67c8bb0ceaf649 Mon Sep 17 00:00:00 2001 From: Stephanie Hingtgen Date: Thu, 29 May 2025 01:40:06 -0500 Subject: [PATCH] Org: Fix org deletion (#106193) --- .../folder/folderimpl/sqlstore_test.go | 18 ++++- pkg/services/org/orgimpl/org_delete_svc.go | 27 +++++++- .../org/orgimpl/org_delete_svc_test.go | 67 +++++++++++++++++++ 3 files changed, 108 insertions(+), 4 deletions(-) create mode 100644 pkg/services/org/orgimpl/org_delete_svc_test.go diff --git a/pkg/services/folder/folderimpl/sqlstore_test.go b/pkg/services/folder/folderimpl/sqlstore_test.go index b049cf43880..4f5b915ac48 100644 --- a/pkg/services/folder/folderimpl/sqlstore_test.go +++ b/pkg/services/folder/folderimpl/sqlstore_test.go @@ -14,8 +14,10 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/folder" "github.com/grafana/grafana/pkg/services/org" @@ -997,16 +999,28 @@ func TestIntegrationGetFolders(t *testing.T) { func CreateOrg(t *testing.T, db db.DB, cfg *setting.Cfg) int64 { t.Helper() + requester := &identity.StaticRequester{ + OrgID: 1, + Permissions: map[int64]map[string][]string{ + 1: map[string][]string{ + accesscontrol.ActionOrgsDelete: {"*"}, + }, + 2: map[string][]string{ + accesscontrol.ActionOrgsDelete: {"*"}, + }, + }, + } orgService, err := orgimpl.ProvideService(db, cfg, quotatest.New(false, nil)) require.NoError(t, err) dashSvc := &dashboards.FakeDashboardService{} dashSvc.On("DeleteAllDashboards", mock.Anything, mock.Anything).Return(nil) - deleteOrgService, err := orgimpl.ProvideDeletionService(db, cfg, dashSvc) + deleteOrgService, err := orgimpl.ProvideDeletionService(db, cfg, dashSvc, acimpl.ProvideAccessControlTest()) require.NoError(t, err) orgID, err := orgService.GetOrCreate(context.Background(), "test-org") require.NoError(t, err) t.Cleanup(func() { - err = deleteOrgService.Delete(context.Background(), &org.DeleteOrgCommand{ID: orgID}) + ctx := identity.WithRequester(context.Background(), requester) + err = deleteOrgService.Delete(ctx, &org.DeleteOrgCommand{ID: orgID}) require.NoError(t, err) }) diff --git a/pkg/services/org/orgimpl/org_delete_svc.go b/pkg/services/org/orgimpl/org_delete_svc.go index e2aba89fc50..4ea185aaef5 100644 --- a/pkg/services/org/orgimpl/org_delete_svc.go +++ b/pkg/services/org/orgimpl/org_delete_svc.go @@ -2,9 +2,12 @@ package orgimpl import ( "context" + "errors" + "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/dashboards" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/setting" @@ -15,9 +18,10 @@ type DeletionService struct { cfg *setting.Cfg log log.Logger dashSvc dashboards.DashboardService + ac accesscontrol.AccessControl } -func ProvideDeletionService(db db.DB, cfg *setting.Cfg, dashboardService dashboards.DashboardService) (org.DeletionService, error) { +func ProvideDeletionService(db db.DB, cfg *setting.Cfg, dashboardService dashboards.DashboardService, ac accesscontrol.AccessControl) (org.DeletionService, error) { log := log.New("org deletion service") s := &DeletionService{ store: &sqlStore{ @@ -28,13 +32,32 @@ func ProvideDeletionService(db db.DB, cfg *setting.Cfg, dashboardService dashboa cfg: cfg, dashSvc: dashboardService, log: log, + ac: ac, } return s, nil } func (s *DeletionService) Delete(ctx context.Context, cmd *org.DeleteOrgCommand) error { - err := s.dashSvc.DeleteAllDashboards(ctx, cmd.ID) + // we need to use a service identity to delete dashboards from the dashboard service (because the currently signed in user + // has to be signed into a different org to delete another org, and so this will fail the namespace check). While we already + // do auth checks on the /api layer, since this is available on the service, adding a check here as well to be safe, in case any additional + // usage is added internally. + requester, err := identity.GetRequester(ctx) + if err != nil { + return err + } + + hasAccess, err := s.ac.Evaluate(ctx, requester, accesscontrol.EvalPermission(accesscontrol.ActionOrgsDelete)) + if err != nil { + return err + } + if !hasAccess { + return errors.New("access denied to delete org") + } + + ctx, _ = identity.WithServiceIdentity(ctx, cmd.ID) + err = s.dashSvc.DeleteAllDashboards(ctx, cmd.ID) if err != nil { return err } diff --git a/pkg/services/org/orgimpl/org_delete_svc_test.go b/pkg/services/org/orgimpl/org_delete_svc_test.go new file mode 100644 index 00000000000..bcc966a1b6d --- /dev/null +++ b/pkg/services/org/orgimpl/org_delete_svc_test.go @@ -0,0 +1,67 @@ +package orgimpl + +import ( + "context" + "testing" + + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" + "github.com/grafana/grafana/pkg/services/dashboards" + "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/setting" +) + +func TestDeletionService_Delete(t *testing.T) { + store := &FakeOrgStore{} + ac := acimpl.ProvideAccessControl(featuremgmt.WithFeatures()) + dashSvc := dashboards.NewFakeDashboardService(t) + svc := &DeletionService{ + store: store, + cfg: setting.NewCfg(), + dashSvc: dashSvc, + ac: ac, + } + + // if a user has access to delete orgs, then the dashboards should be deleted with a service identity + requester := &identity.StaticRequester{ + OrgID: 1, + Permissions: map[int64]map[string][]string{ + 1: map[string][]string{ + accesscontrol.ActionOrgsDelete: {"*"}, + }, + 2: map[string][]string{ + accesscontrol.ActionOrgsDelete: {"*"}, + }, + }, + } + dashSvc.On("DeleteAllDashboards", mock.MatchedBy(func(ctx context.Context) bool { + return identity.IsServiceIdentity(ctx) + }), int64(2)).Return(nil).Once() + ctx := context.Background() + ctx = identity.WithRequester(ctx, requester) + err := svc.Delete(ctx, &org.DeleteOrgCommand{ID: 2}) + require.NoError(t, err) + dashSvc.AssertExpectations(t) + + // if a user does not have access to delete orgs, then the dashboards should not be deleted + requester = &identity.StaticRequester{ + OrgID: 1, + Permissions: map[int64]map[string][]string{ + 1: map[string][]string{ + accesscontrol.ActionOrgsRead: {"*"}, + }, + 2: map[string][]string{ + accesscontrol.ActionOrgsRead: {"*"}, + }, + }, + } + ctx = context.Background() + ctx = identity.WithRequester(ctx, requester) + err = svc.Delete(ctx, &org.DeleteOrgCommand{ID: 2}) + require.Error(t, err) +}