diff --git a/pkg/services/serviceaccounts/manager/service.go b/pkg/services/serviceaccounts/manager/service.go index b225cc6f0a1..5fbae6c88ef 100644 --- a/pkg/services/serviceaccounts/manager/service.go +++ b/pkg/services/serviceaccounts/manager/service.go @@ -26,7 +26,9 @@ const ( ) type ServiceAccountsService struct { - acService accesscontrol.Service + acService accesscontrol.Service + permissions accesscontrol.ServiceAccountPermissionsService + store store log log.Logger backgroundLog log.Logger @@ -44,7 +46,8 @@ func ProvideServiceAccountsService( kvStore kvstore.KVStore, userService user.Service, orgService org.Service, - accesscontrolService accesscontrol.Service, + acService accesscontrol.Service, + permissions accesscontrol.ServiceAccountPermissionsService, ) (*ServiceAccountsService, error) { serviceAccountsStore := database.ProvideServiceAccountsStore( cfg, @@ -55,13 +58,14 @@ func ProvideServiceAccountsService( orgService, ) s := &ServiceAccountsService{ - acService: accesscontrolService, + acService: acService, + permissions: permissions, store: serviceAccountsStore, log: log.New("serviceaccounts"), backgroundLog: log.New("serviceaccounts.background"), } - if err := RegisterRoles(accesscontrolService); err != nil { + if err := RegisterRoles(acService); err != nil { s.log.Error("Failed to register roles", "error", err) } @@ -179,7 +183,10 @@ func (sa *ServiceAccountsService) DeleteServiceAccount(ctx context.Context, orgI if err := sa.store.DeleteServiceAccount(ctx, orgID, serviceAccountID); err != nil { return err } - return sa.acService.DeleteUserPermissions(ctx, orgID, serviceAccountID) + if err := sa.acService.DeleteUserPermissions(ctx, orgID, serviceAccountID); err != nil { + return err + } + return sa.permissions.DeleteResourcePermissions(ctx, orgID, fmt.Sprintf("%d", serviceAccountID)) } func (sa *ServiceAccountsService) EnableServiceAccount(ctx context.Context, orgID, serviceAccountID int64, enable bool) error { diff --git a/pkg/services/serviceaccounts/manager/service_test.go b/pkg/services/serviceaccounts/manager/service_test.go index 9345235039e..579ee7ff598 100644 --- a/pkg/services/serviceaccounts/manager/service_test.go +++ b/pkg/services/serviceaccounts/manager/service_test.go @@ -119,7 +119,8 @@ func (f *SecretsCheckerFake) CheckTokens(ctx context.Context) error { func TestProvideServiceAccount_DeleteServiceAccount(t *testing.T) { storeMock := newServiceAccountStoreFake() acSvc := actest.FakeService{} - svc := ServiceAccountsService{acSvc, storeMock, log.New("test"), log.New("background.test"), &SecretsCheckerFake{}, false, 0} + pSvc := &actest.FakePermissionsService{} + svc := ServiceAccountsService{acSvc, pSvc, storeMock, log.NewNopLogger(), log.NewNopLogger(), &SecretsCheckerFake{}, false, 0} testOrgId := 1 t.Run("should create service account", func(t *testing.T) { diff --git a/pkg/services/serviceaccounts/manager/stats_test.go b/pkg/services/serviceaccounts/manager/stats_test.go index e85e9adb129..43698a18494 100644 --- a/pkg/services/serviceaccounts/manager/stats_test.go +++ b/pkg/services/serviceaccounts/manager/stats_test.go @@ -14,8 +14,9 @@ import ( func Test_UsageStats(t *testing.T) { acSvc := actest.FakeService{} + pSvc := actest.FakePermissionsService{} storeMock := newServiceAccountStoreFake() - svc := ServiceAccountsService{acSvc, storeMock, log.New("test"), log.New("background-test"), &SecretsCheckerFake{}, true, 5} + svc := ServiceAccountsService{acSvc, &pSvc, storeMock, log.NewNopLogger(), log.NewNopLogger(), &SecretsCheckerFake{}, true, 5} err := svc.DeleteServiceAccount(context.Background(), 1, 1) require.NoError(t, err) diff --git a/pkg/services/sqlstore/migrations/accesscontrol/orphaned.go b/pkg/services/sqlstore/migrations/accesscontrol/orphaned.go new file mode 100644 index 00000000000..05ade14da83 --- /dev/null +++ b/pkg/services/sqlstore/migrations/accesscontrol/orphaned.go @@ -0,0 +1,96 @@ +package accesscontrol + +import ( + "fmt" + "strconv" + "strings" + + "xorm.io/xorm" + + "github.com/grafana/grafana/pkg/services/sqlstore/migrator" +) + +const ( + orphanedServiceAccountsPermissions = "delete orphaned service account permissions" +) + +func AddOrphanedMigrations(mg *migrator.Migrator) { + mg.AddMigration(orphanedServiceAccountsPermissions, &orphanedServiceAccountPermissions{}) +} + +var _ migrator.CodeMigration = new(alertingScopeRemovalMigrator) + +type orphanedServiceAccountPermissions struct { + migrator.MigrationBase +} + +func (m *orphanedServiceAccountPermissions) SQL(dialect migrator.Dialect) string { + return CodeMigrationSQL +} + +func (m *orphanedServiceAccountPermissions) Exec(sess *xorm.Session, mg *migrator.Migrator) error { + var idents []string + + // find all permissions that are scopes directly to a service account + err := sess.SQL("SELECT DISTINCT p.identifier FROM permission AS p WHERE p.kind = 'serviceaccounts' AND NOT p.identifier = '*'").Find(&idents) + if err != nil { + return fmt.Errorf("failed to fetch permissinos scoped to service accounts: %w", err) + } + + ids := make([]int64, 0, len(idents)) + for _, id := range idents { + id, err := strconv.ParseInt(id, 10, 64) + if err == nil { + ids = append(ids, id) + } + } + + if len(ids) == 0 { + return nil + } + + // Then find all existing service accounts + raw := "SELECT u.id FROM " + mg.Dialect.Quote("user") + " AS u WHERE u.is_service_account AND u.id IN(?" + strings.Repeat(",?", len(ids)-1) + ")" + args := make([]any, 0, len(ids)) + for _, id := range ids { + args = append(args, id) + } + + var existingIDs []int64 + err = sess.SQL(raw, args...).Find(&existingIDs) + if err != nil { + return fmt.Errorf("failed to fetch existing service accounts: %w", err) + } + + existing := make(map[int64]struct{}, len(existingIDs)) + for _, id := range existingIDs { + existing[id] = struct{}{} + } + + // filter out orphaned permissions + var orphaned []string + for _, id := range ids { + if _, ok := existing[id]; !ok { + orphaned = append(orphaned, strconv.FormatInt(id, 10)) + } + } + + if len(orphaned) == 0 { + return nil + } + + // delete all orphaned permissions + rawDelete := "DELETE FROM permission AS p WHERE p.kind = 'serviceaccounts' AND p.identifier IN(?" + strings.Repeat(",?", len(orphaned)-1) + ")" + deleteArgs := make([]any, 0, len(orphaned)+1) + deleteArgs = append(deleteArgs, rawDelete) + for _, id := range orphaned { + deleteArgs = append(deleteArgs, id) + } + + _, err = sess.Exec(deleteArgs...) + if err != nil { + return fmt.Errorf("failed to delete orphaned service accounts: %w", err) + } + + return nil +} diff --git a/pkg/services/sqlstore/migrations/migrations.go b/pkg/services/sqlstore/migrations/migrations.go index 216f2ad6f89..f8de7ec326e 100644 --- a/pkg/services/sqlstore/migrations/migrations.go +++ b/pkg/services/sqlstore/migrations/migrations.go @@ -131,6 +131,8 @@ func (oss *OSSMigrations) AddMigration(mg *Migrator) { ualert.AddReceiverActionScopesMigration(mg) ualert.AddRuleMetadata(mg) + + accesscontrol.AddOrphanedMigrations(mg) } func addStarMigrations(mg *Migrator) {