From 421a215340a6962a5e3493d2c81d9455313646b4 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Tue, 30 Jun 2026 16:56:04 +0530 Subject: [PATCH 1/8] feat: GitOps platform-user management (bootstrap superuser SA + reconcile CLI + hardening) --- cmd/reconcile.go | 76 +++++++ cmd/root.go | 1 + cmd/serve.go | 9 +- config/sample.config.yaml | 18 +- internal/api/v1beta1connect/platform.go | 7 + internal/api/v1beta1connect/platform_test.go | 13 ++ internal/api/v1beta1connect/serviceuser.go | 28 +++ .../api/v1beta1connect/serviceuser_test.go | 49 +++++ internal/bootstrap/bootstrapuser.go | 199 ++++++++++++++++++ internal/bootstrap/bootstrapuser_test.go | 183 ++++++++++++++++ internal/bootstrap/schema/schema.go | 7 + internal/bootstrap/service.go | 37 ++-- internal/reconcile/platformuser.go | 150 +++++++++++++ internal/reconcile/platformuser_reconciler.go | 159 ++++++++++++++ .../reconcile/platformuser_reconciler_test.go | 151 +++++++++++++ internal/reconcile/platformuser_test.go | 129 ++++++++++++ internal/reconcile/reconcile.go | 70 ++++++ .../store/postgres/serviceuser_repository.go | 8 +- test/e2e/testbench/helper.go | 37 ++++ test/e2e/testbench/testbench.go | 13 +- 20 files changed, 1308 insertions(+), 36 deletions(-) create mode 100644 cmd/reconcile.go create mode 100644 internal/bootstrap/bootstrapuser.go create mode 100644 internal/bootstrap/bootstrapuser_test.go create mode 100644 internal/reconcile/platformuser.go create mode 100644 internal/reconcile/platformuser_reconciler.go create mode 100644 internal/reconcile/platformuser_reconciler_test.go create mode 100644 internal/reconcile/platformuser_test.go create mode 100644 internal/reconcile/reconcile.go diff --git a/cmd/reconcile.go b/cmd/reconcile.go new file mode 100644 index 000000000..a4ddc8566 --- /dev/null +++ b/cmd/reconcile.go @@ -0,0 +1,76 @@ +package cmd + +import ( + "fmt" + "os" + + "github.com/MakeNowJust/heredoc" + "github.com/raystack/frontier/internal/reconcile" + cli "github.com/spf13/cobra" +) + +func ReconcileCommand(cliConfig *Config) *cli.Command { + var ( + filePath string + dryRun bool + header string + ) + cmd := &cli.Command{ + Use: "reconcile", + Short: "Reconcile declarative platform configuration to a desired-state file", + Long: heredoc.Doc(` + Converge platform resources to a declarative YAML spec via the admin API. + + Currently supports the PlatformUser kind (platform admins/members). The file + is the source of truth: entries present are ensured, entries absent are removed. + Authenticate as a superuser (e.g. the bootstrap service account) via --header. + `), + Example: heredoc.Doc(` + $ frontier reconcile -f platform-users.yaml --dry-run -H "Authorization:Basic " + $ frontier reconcile -f platform-users.yaml -H "Authorization:Basic " + `), + Annotations: map[string]string{ + "group": "core", + "client": "true", + }, + RunE: func(cmd *cli.Command, args []string) error { + data, err := os.ReadFile(filePath) + if err != nil { + return fmt.Errorf("read desired-state file: %w", err) + } + adminClient, err := createAdminClient(cliConfig.Host) + if err != nil { + return err + } + registry := map[string]reconcile.Reconciler{ + reconcile.KindPlatformUser: reconcile.NewPlatformUserReconciler(adminClient, header), + } + reports, runErr := reconcile.Run(cmd.Context(), registry, data, dryRun) + for _, rep := range reports { + printReconcileReport(cmd, rep) + } + return runErr + }, + } + cmd.Flags().StringVarP(&filePath, "file", "f", "", "Path to the desired-state YAML file") + cmd.MarkFlagRequired("file") + cmd.Flags().BoolVar(&dryRun, "dry-run", false, "Print the plan without applying changes") + cmd.Flags().StringVarP(&header, "header", "H", "", "Header : for auth, e.g. 'Authorization:Basic '") + bindFlagsFromClientConfig(cmd) + return cmd +} + +func printReconcileReport(cmd *cli.Command, rep reconcile.Report) { + if len(rep.Planned) == 0 { + cmd.Printf("%s: no changes\n", rep.Kind) + return + } + verb := "applied" + if rep.DryRun { + verb = "planned" + } + cmd.Printf("%s (%s %d):\n", rep.Kind, verb, len(rep.Planned)) + for _, p := range rep.Planned { + cmd.Printf(" - %s\n", p) + } +} diff --git a/cmd/root.go b/cmd/root.go index 8d1fb6180..552b811a3 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -49,6 +49,7 @@ func New(cliConfig *Config) *cli.Command { cmd.AddCommand(PermissionCommand(cliConfig)) cmd.AddCommand(PolicyCommand(cliConfig)) cmd.AddCommand(SeedCommand(cliConfig)) + cmd.AddCommand(ReconcileCommand(cliConfig)) cmd.AddCommand(configCommand()) cmd.AddCommand(versionCommand()) cmd.AddCommand(PreferencesCommand(cliConfig)) diff --git a/cmd/serve.go b/cmd/serve.go index 4982a6dcb..3bbf77820 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -212,8 +212,9 @@ func StartServer(logger *slog.Logger, cfg *config.Frontier) error { if err = deps.BootstrapService.MigrateRoles(ctx); err != nil { return err } - // promote normal users to superusers - if err = deps.BootstrapService.MakeSuperUsers(ctx); err != nil { + // ensure the config-seeded bootstrap superuser service account (for automation/GitOps). + // all other platform-user management is handled out-of-band via the GitOps reconcile flow. + if err = deps.BootstrapService.EnsureBootstrapSuperUser(ctx); err != nil { return err } @@ -569,7 +570,6 @@ func buildAPIDependencies( namespaceService, roleService, permissionService, - userService, authzSchemaRepository, relationService, policyService, @@ -577,6 +577,9 @@ func buildAPIDependencies( cfg.App.PAT.DeniedPermissionsSet(), planService, planBlobRepository, + svUserRepo, + scUserCredRepo, + serviceUserService, ) cascadeDeleter := deleter.NewCascadeDeleter(organizationService, projectService, resourceService, diff --git a/config/sample.config.yaml b/config/sample.config.yaml index a90840e71..f29974a61 100644 --- a/config/sample.config.yaml +++ b/config/sample.config.yaml @@ -179,12 +179,18 @@ app: # platform level administration admin: - # Email list of users which needs to be converted as superusers - # if the user is already present in the system, it is promoted to su - # if not, a new account is created with provided email id and promoted to su. - # UUIDs/slugs of existing users can also be provided instead of email ids - # but in that case a new user will not be created. - users: [] + # bootstrap seeds a superuser SERVICE ACCOUNT from config (a username/password-style + # client_id + client_secret) so automation like the GitOps reconcile flow has a + # guaranteed superuser identity without a chicken-and-egg. The account is ensured and + # promoted to superuser on every boot (idempotent); the secret is rotated if it changes + # here. Authenticate with: Authorization: Basic base64(client_id:client_secret). + # Leave client_id/client_secret empty to disable. + # client_id must be a UUID (it is the service-account credential id); generate one + # (e.g. uuidgen) and keep it stable. client_secret is your chosen password. + bootstrap: + client_id: "" + client_secret: "" + # title: "GitOps Bootstrap Superuser" # smtp configuration for sending emails mailer: smtp_host: smtp.example.com diff --git a/internal/api/v1beta1connect/platform.go b/internal/api/v1beta1connect/platform.go index 4066a1547..9d9d4d915 100644 --- a/internal/api/v1beta1connect/platform.go +++ b/internal/api/v1beta1connect/platform.go @@ -51,6 +51,13 @@ func (h *ConnectHandler) RemovePlatformUser(ctx context.Context, req *connect.Re } } } else if req.Msg.GetServiceuserId() != "" { + // Protect the config-bootstrapped break-glass SA (well-known id). It is + // seeded and managed at boot, not via this API, while reconcile is + // authoritative over service accounts — without this guard an apply (or a + // stray call) would strip its superuser access until the next restart. + if req.Msg.GetServiceuserId() == schema.BootstrapServiceUserID { + return nil, connect.NewError(connect.CodePermissionDenied, fmt.Errorf("cannot remove the bootstrap superuser service account")) + } for _, relationName := range platformRelations { if err := h.serviceUserService.UnSudo(ctx, req.Msg.GetServiceuserId(), relationName); err != nil { return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("RemovePlatformUser.ServiceUserUnSudo: service_user_id=%s relation=%s: %w", req.Msg.GetServiceuserId(), relationName, err)) diff --git a/internal/api/v1beta1connect/platform_test.go b/internal/api/v1beta1connect/platform_test.go index a4ef6a446..2de2e4bac 100644 --- a/internal/api/v1beta1connect/platform_test.go +++ b/internal/api/v1beta1connect/platform_test.go @@ -43,6 +43,19 @@ func TestHandler_RemovePlatformUser(t *testing.T) { assert.NotNil(t, resp) }) + t.Run("refuses to remove the bootstrap superuser service account", func(t *testing.T) { + serviceUserSvc := mocks.NewServiceUserService(t) + // the target is the well-known bootstrap SA -> reject before any UnSudo. + h := &ConnectHandler{serviceUserService: serviceUserSvc} + resp, err := h.RemovePlatformUser(context.Background(), connect.NewRequest(&frontierv1beta1.RemovePlatformUserRequest{ + ServiceuserId: schema.BootstrapServiceUserID, + })) + assert.Error(t, err) + assert.Nil(t, resp) + assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + serviceUserSvc.AssertNotCalled(t, "UnSudo", mock.Anything, mock.Anything, mock.Anything) + }) + t.Run("removes only the specified relation when relation is set", func(t *testing.T) { userSvc := mocks.NewUserService(t) // only the admin relation is stripped; an UnSudo for member would be an diff --git a/internal/api/v1beta1connect/serviceuser.go b/internal/api/v1beta1connect/serviceuser.go index 653297be3..e9c38d984 100644 --- a/internal/api/v1beta1connect/serviceuser.go +++ b/internal/api/v1beta1connect/serviceuser.go @@ -168,11 +168,27 @@ func (h *ConnectHandler) CreateServiceUser(ctx context.Context, request *connect }), nil } +// errBootstrapSAImmutable returns a PermissionDenied error when id is the +// config-bootstrapped break-glass SA (well-known id). That account is managed +// only via app.admin.bootstrap at boot — the API must never delete it or mint +// credentials/keys/tokens for it (which would create a persistent, rotation-proof +// superuser backdoor). Not even platform superusers are allowed. +func errBootstrapSAImmutable(id string) error { + if id == schema.BootstrapServiceUserID { + return connect.NewError(connect.CodePermissionDenied, fmt.Errorf("the bootstrap superuser service account is managed via app.admin.bootstrap and cannot be modified through the API")) + } + return nil +} + func (h *ConnectHandler) DeleteServiceUser(ctx context.Context, request *connect.Request[frontierv1beta1.DeleteServiceUserRequest]) (*connect.Response[frontierv1beta1.DeleteServiceUserResponse], error) { errorLogger := NewErrorLogger() serviceUserID := request.Msg.GetId() orgID := request.Msg.GetOrgId() + if err := errBootstrapSAImmutable(serviceUserID); err != nil { + return nil, err + } + err := h.serviceUserService.Delete(ctx, serviceUserID) if err != nil { errorLogger.LogServiceError(ctx, request, "DeleteServiceUser", err, @@ -198,6 +214,10 @@ func (h *ConnectHandler) CreateServiceUserJWK(ctx context.Context, request *conn serviceUserID := request.Msg.GetId() title := request.Msg.GetTitle() + if err := errBootstrapSAImmutable(serviceUserID); err != nil { + return nil, err + } + svCred, err := h.serviceUserService.CreateKey(ctx, serviceuser.Credential{ ServiceUserID: serviceUserID, Title: title, @@ -312,6 +332,10 @@ func (h *ConnectHandler) CreateServiceUserCredential(ctx context.Context, reques serviceUserID := request.Msg.GetId() title := request.Msg.GetTitle() + if err := errBootstrapSAImmutable(serviceUserID); err != nil { + return nil, err + } + secret, err := h.serviceUserService.CreateSecret(ctx, serviceuser.Credential{ ServiceUserID: serviceUserID, Title: title, @@ -364,6 +388,10 @@ func (h *ConnectHandler) CreateServiceUserToken(ctx context.Context, request *co serviceUserID := request.Msg.GetId() title := request.Msg.GetTitle() + if err := errBootstrapSAImmutable(serviceUserID); err != nil { + return nil, err + } + secret, err := h.serviceUserService.CreateToken(ctx, serviceuser.Credential{ ServiceUserID: serviceUserID, Title: title, diff --git a/internal/api/v1beta1connect/serviceuser_test.go b/internal/api/v1beta1connect/serviceuser_test.go index be83cfe2c..5daa1ca3c 100644 --- a/internal/api/v1beta1connect/serviceuser_test.go +++ b/internal/api/v1beta1connect/serviceuser_test.go @@ -1012,6 +1012,55 @@ func TestHandler_ListServiceUserCredentials(t *testing.T) { } } +func TestHandler_BootstrapSAImmutable(t *testing.T) { + // The bootstrap SA (well-known id) must be immutable via the API: no delete and + // no minting of credentials/keys/tokens (which would be a rotation-proof + // superuser backdoor). Each guard must reject before touching the service. + t.Run("DeleteServiceUser is refused", func(t *testing.T) { + su := new(mocks.ServiceUserService) + h := &ConnectHandler{serviceUserService: su} + resp, err := h.DeleteServiceUser(context.Background(), connect.NewRequest(&frontierv1beta1.DeleteServiceUserRequest{ + Id: schema.BootstrapServiceUserID, + })) + assert.Nil(t, resp) + assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + su.AssertNotCalled(t, "Delete", mock.Anything, mock.Anything) + }) + + t.Run("CreateServiceUserCredential is refused", func(t *testing.T) { + su := new(mocks.ServiceUserService) + h := &ConnectHandler{serviceUserService: su} + resp, err := h.CreateServiceUserCredential(context.Background(), connect.NewRequest(&frontierv1beta1.CreateServiceUserCredentialRequest{ + Id: schema.BootstrapServiceUserID, + })) + assert.Nil(t, resp) + assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + su.AssertNotCalled(t, "CreateSecret", mock.Anything, mock.Anything) + }) + + t.Run("CreateServiceUserToken is refused", func(t *testing.T) { + su := new(mocks.ServiceUserService) + h := &ConnectHandler{serviceUserService: su} + resp, err := h.CreateServiceUserToken(context.Background(), connect.NewRequest(&frontierv1beta1.CreateServiceUserTokenRequest{ + Id: schema.BootstrapServiceUserID, + })) + assert.Nil(t, resp) + assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + su.AssertNotCalled(t, "CreateToken", mock.Anything, mock.Anything) + }) + + t.Run("CreateServiceUserJWK is refused", func(t *testing.T) { + su := new(mocks.ServiceUserService) + h := &ConnectHandler{serviceUserService: su} + resp, err := h.CreateServiceUserJWK(context.Background(), connect.NewRequest(&frontierv1beta1.CreateServiceUserJWKRequest{ + Id: schema.BootstrapServiceUserID, + })) + assert.Nil(t, resp) + assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + su.AssertNotCalled(t, "CreateKey", mock.Anything, mock.Anything) + }) +} + func TestHandler_DeleteServiceUserCredential(t *testing.T) { tests := []struct { name string diff --git a/internal/bootstrap/bootstrapuser.go b/internal/bootstrap/bootstrapuser.go new file mode 100644 index 000000000..bec875cd0 --- /dev/null +++ b/internal/bootstrap/bootstrapuser.go @@ -0,0 +1,199 @@ +package bootstrap + +import ( + "context" + "errors" + "fmt" + "log/slog" + "strings" + + "github.com/google/uuid" + "golang.org/x/crypto/bcrypt" + + "github.com/raystack/frontier/core/serviceuser" + "github.com/raystack/frontier/internal/bootstrap/schema" +) + +// bootstrapBcryptCost matches serviceuser.CreateSecret's cost for client secrets. +// It is a var (not a const) so unit tests can lower it to bcrypt.MinCost — cost 14 +// is deliberately expensive and, run under `-race -count 2`, blows the test timeout. +var bootstrapBcryptCost = 14 + +const defaultBootstrapTitle = "GitOps Bootstrap Superuser" + +// SuperUserBootstrapConfig configures a config-seeded superuser service account. +// It supplies a username/password-style credential (client_id + client_secret) +// so automation (e.g. the GitOps reconcile flow) has a guaranteed superuser +// identity at boot — removing the chicken-and-egg of needing a superuser to +// create the first superuser. The secret is operator-provided and rotated when +// it changes. Leave client_id/client_secret empty to disable. +type SuperUserBootstrapConfig struct { + ClientID string `yaml:"client_id" mapstructure:"client_id"` + ClientSecret string `yaml:"client_secret" mapstructure:"client_secret"` + Title string `yaml:"title" mapstructure:"title"` +} + +func (c SuperUserBootstrapConfig) enabled() bool { + return strings.TrimSpace(c.ClientID) != "" && c.ClientSecret != "" +} + +func (c SuperUserBootstrapConfig) title() string { + if t := strings.TrimSpace(c.Title); t != "" { + return t + } + return defaultBootstrapTitle +} + +// ServiceUserCreator creates a bare service-user row. The bootstrap superuser +// deliberately skips org membership (a platform superuser needs none), so this +// is the repository's Create, not serviceuser.Service.Create. +type ServiceUserCreator interface { + Create(ctx context.Context, su serviceuser.ServiceUser) (serviceuser.ServiceUser, error) + // Delete removes a service user by id. Used to roll back a just-created + // bootstrap service user when credential creation fails, so repeated boot + // failures don't accumulate credential-less orphan rows. + Delete(ctx context.Context, id string) error +} + +// ServiceUserCredentialStore manages client-secret credentials keyed by id. +type ServiceUserCredentialStore interface { + Get(ctx context.Context, id string) (serviceuser.Credential, error) + Create(ctx context.Context, cred serviceuser.Credential) (serviceuser.Credential, error) + Delete(ctx context.Context, id string) error +} + +// SuperUserPromoter grants a platform relation (admin/member) to a principal. +type SuperUserPromoter interface { + Sudo(ctx context.Context, id, relationName string) error +} + +// EnsureBootstrapSuperUser idempotently ensures the configured superuser service +// account exists and is a platform superuser. No-op when not configured. +func (s Service) EnsureBootstrapSuperUser(ctx context.Context) error { + return ensureBootstrapSuperUser(ctx, s.logger, s.adminConfig.Bootstrap, s.suCreator, s.suCredStore, s.suPromoter) +} + +// ensureBootstrapSuperUser holds the testable core. The account lives in the +// platform/nil org (serviceusers.org_id is nullable, no FK) and is created +// without org membership. Idempotency is keyed on the credential id (client_id): +// - absent: create the service user + client-secret credential + promote; +// - present: rotate the secret if it changed, then (re)ensure the superuser relation. +func ensureBootstrapSuperUser( + ctx context.Context, + logger *slog.Logger, + cfg SuperUserBootstrapConfig, + users ServiceUserCreator, + creds ServiceUserCredentialStore, + promoter SuperUserPromoter, +) error { + if !cfg.enabled() { + return nil + } + clientID := strings.TrimSpace(cfg.ClientID) + // client_id is the service-user credential's id, which is a UUID column; a + // non-UUID would otherwise fail with an opaque SQL error deep in the lookup. + if _, err := uuid.Parse(clientID); err != nil { + return fmt.Errorf("bootstrap superuser: client_id must be a valid uuid: %w", err) + } + + cred, err := creds.Get(ctx, clientID) + switch { + case errors.Is(err, serviceuser.ErrCredNotExist): + return createBootstrapSuperUser(ctx, logger, cfg, clientID, users, creds, promoter) + case err != nil: + return fmt.Errorf("bootstrap superuser: get credential %q: %w", clientID, err) + } + + if bcrypt.CompareHashAndPassword([]byte(cred.SecretHash), []byte(cfg.ClientSecret)) != nil { + if err := rotateBootstrapSecret(ctx, cfg, clientID, cred, creds); err != nil { + return err + } + logger.InfoContext(ctx, "rotated bootstrap superuser secret", "client_id", clientID) + } + return promoteBootstrapSuperUser(ctx, promoter, cred.ServiceUserID) +} + +func createBootstrapSuperUser( + ctx context.Context, + logger *slog.Logger, + cfg SuperUserBootstrapConfig, + clientID string, + users ServiceUserCreator, + creds ServiceUserCredentialStore, + promoter SuperUserPromoter, +) error { + su, err := users.Create(ctx, serviceuser.ServiceUser{ + ID: schema.BootstrapServiceUserID, // fixed, well-known id (see schema.BootstrapServiceUserID) + OrgID: schema.PlatformOrgID.String(), + Title: cfg.title(), + }) + if err != nil { + return fmt.Errorf("bootstrap superuser: create service user: %w", err) + } + hash, err := bcrypt.GenerateFromPassword([]byte(cfg.ClientSecret), bootstrapBcryptCost) + if err != nil { + return cleanupOrphanBootstrapSU(ctx, logger, users, su.ID, + fmt.Errorf("bootstrap superuser: hash secret: %w", err)) + } + if _, err := creds.Create(ctx, serviceuser.Credential{ + ID: clientID, + ServiceUserID: su.ID, + Type: serviceuser.ClientSecretCredentialType, + SecretHash: string(hash), + Title: cfg.title(), + }); err != nil { + return cleanupOrphanBootstrapSU(ctx, logger, users, su.ID, + fmt.Errorf("bootstrap superuser: create credential: %w", err)) + } + logger.InfoContext(ctx, "created bootstrap superuser service account", + "client_id", clientID, "serviceuser_id", su.ID) + return promoteBootstrapSuperUser(ctx, promoter, su.ID) +} + +// cleanupOrphanBootstrapSU best-effort deletes a service user created moments +// earlier when a later step (hashing, credential creation) fails. Without it the +// row would be orphaned — credential-less and unpromoted — and the next boot, +// still finding no credential, would create another. The original cause is always +// returned; a failed cleanup is only logged (the next boot retries anyway). +func cleanupOrphanBootstrapSU(ctx context.Context, logger *slog.Logger, users ServiceUserCreator, suID string, cause error) error { + if err := users.Delete(ctx, suID); err != nil { + logger.WarnContext(ctx, "failed to roll back orphan bootstrap service user", + "serviceuser_id", suID, "err", err.Error()) + } + return cause +} + +// rotateBootstrapSecret replaces the stored secret. The credential repo has no +// update, so delete + recreate with the same id (a brief gap at boot is fine). +func rotateBootstrapSecret( + ctx context.Context, + cfg SuperUserBootstrapConfig, + clientID string, + cred serviceuser.Credential, + creds ServiceUserCredentialStore, +) error { + hash, err := bcrypt.GenerateFromPassword([]byte(cfg.ClientSecret), bootstrapBcryptCost) + if err != nil { + return fmt.Errorf("bootstrap superuser: hash secret: %w", err) + } + if err := creds.Delete(ctx, clientID); err != nil { + return fmt.Errorf("bootstrap superuser: rotate (delete): %w", err) + } + if _, err := creds.Create(ctx, serviceuser.Credential{ + ID: clientID, + ServiceUserID: cred.ServiceUserID, + Type: serviceuser.ClientSecretCredentialType, + SecretHash: string(hash), + Title: cred.Title, + }); err != nil { + return fmt.Errorf("bootstrap superuser: rotate (create): %w", err) + } + return nil +} + +func promoteBootstrapSuperUser(ctx context.Context, promoter SuperUserPromoter, suID string) error { + if err := promoter.Sudo(ctx, suID, schema.AdminRelationName); err != nil { + return fmt.Errorf("bootstrap superuser: promote %q: %w", suID, err) + } + return nil +} diff --git a/internal/bootstrap/bootstrapuser_test.go b/internal/bootstrap/bootstrapuser_test.go new file mode 100644 index 000000000..68d2a716b --- /dev/null +++ b/internal/bootstrap/bootstrapuser_test.go @@ -0,0 +1,183 @@ +package bootstrap + +import ( + "context" + "errors" + "io" + "log/slog" + "testing" + + "golang.org/x/crypto/bcrypt" + + "github.com/raystack/frontier/core/serviceuser" + "github.com/raystack/frontier/internal/bootstrap/schema" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +type mockSUCreator struct{ mock.Mock } + +func (m *mockSUCreator) Create(ctx context.Context, su serviceuser.ServiceUser) (serviceuser.ServiceUser, error) { + args := m.Called(ctx, su) + return args.Get(0).(serviceuser.ServiceUser), args.Error(1) +} + +func (m *mockSUCreator) Delete(ctx context.Context, id string) error { + return m.Called(ctx, id).Error(0) +} + +type mockCredStore struct{ mock.Mock } + +func (m *mockCredStore) Get(ctx context.Context, id string) (serviceuser.Credential, error) { + args := m.Called(ctx, id) + return args.Get(0).(serviceuser.Credential), args.Error(1) +} + +func (m *mockCredStore) Create(ctx context.Context, cred serviceuser.Credential) (serviceuser.Credential, error) { + args := m.Called(ctx, cred) + return args.Get(0).(serviceuser.Credential), args.Error(1) +} + +func (m *mockCredStore) Delete(ctx context.Context, id string) error { + return m.Called(ctx, id).Error(0) +} + +type mockSUPromoter struct{ mock.Mock } + +func (m *mockSUPromoter) Sudo(ctx context.Context, id, relationName string) error { + return m.Called(ctx, id, relationName).Error(0) +} + +func bcryptHash(t *testing.T, secret string) string { + t.Helper() + h, err := bcrypt.GenerateFromPassword([]byte(secret), bootstrapBcryptCost) + if err != nil { + t.Fatalf("hash: %v", err) + } + return string(h) +} + +func TestEnsureBootstrapSuperUser(t *testing.T) { + // Cost 14 is intentionally slow; under `-race -count 2` even a handful of + // hash/compare calls blow the 150s test timeout (measured ~151s). Behaviour is + // identical at any cost, so run the unit test at the minimum. + orig := bootstrapBcryptCost + bootstrapBcryptCost = bcrypt.MinCost + t.Cleanup(func() { bootstrapBcryptCost = orig }) + + ctx := context.Background() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + const clientID = "f47ac10b-58cc-4372-a567-0e02b2c3d479" + + t.Run("no-op when not configured", func(t *testing.T) { + users, creds, prom := new(mockSUCreator), new(mockCredStore), new(mockSUPromoter) + + assert.NoError(t, ensureBootstrapSuperUser(ctx, logger, SuperUserBootstrapConfig{}, users, creds, prom)) + creds.AssertNotCalled(t, "Get", mock.Anything, mock.Anything) + users.AssertNotCalled(t, "Create", mock.Anything, mock.Anything) + prom.AssertNotCalled(t, "Sudo", mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("rejects a non-uuid client_id", func(t *testing.T) { + users, creds, prom := new(mockSUCreator), new(mockCredStore), new(mockSUPromoter) + cfg := SuperUserBootstrapConfig{ClientID: "not-a-uuid", ClientSecret: "s3cret"} + + assert.Error(t, ensureBootstrapSuperUser(ctx, logger, cfg, users, creds, prom)) + creds.AssertNotCalled(t, "Get", mock.Anything, mock.Anything) + }) + + t.Run("creates service user + credential + promotes when absent", func(t *testing.T) { + users, creds, prom := new(mockSUCreator), new(mockCredStore), new(mockSUPromoter) + cfg := SuperUserBootstrapConfig{ClientID: clientID, ClientSecret: "s3cret"} + + creds.On("Get", mock.Anything, clientID).Return(serviceuser.Credential{}, serviceuser.ErrCredNotExist) + users.On("Create", mock.Anything, mock.MatchedBy(func(su serviceuser.ServiceUser) bool { + return su.ID == schema.BootstrapServiceUserID && + su.OrgID == schema.PlatformOrgID.String() && su.Title == defaultBootstrapTitle + })).Return(serviceuser.ServiceUser{ID: "su-id"}, nil) + // Capture the credential and verify the hash once after the call rather + // than inside the matcher: testify re-invokes argument matchers, and bcrypt + // is costly enough there to stall CI. + var created serviceuser.Credential + creds.On("Create", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { created = args.Get(1).(serviceuser.Credential) }). + Return(serviceuser.Credential{ID: clientID}, nil) + prom.On("Sudo", mock.Anything, "su-id", schema.AdminRelationName).Return(nil) + + assert.NoError(t, ensureBootstrapSuperUser(ctx, logger, cfg, users, creds, prom)) + users.AssertExpectations(t) + creds.AssertExpectations(t) + prom.AssertExpectations(t) + + assert.Equal(t, clientID, created.ID) + assert.Equal(t, "su-id", created.ServiceUserID) + assert.Equal(t, serviceuser.ClientSecretCredentialType, created.Type) + assert.NoError(t, bcrypt.CompareHashAndPassword([]byte(created.SecretHash), []byte("s3cret"))) + }) + + t.Run("ensures superuser without rotating when the secret matches", func(t *testing.T) { + users, creds, prom := new(mockSUCreator), new(mockCredStore), new(mockSUPromoter) + cfg := SuperUserBootstrapConfig{ClientID: clientID, ClientSecret: "s3cret"} + + creds.On("Get", mock.Anything, clientID).Return(serviceuser.Credential{ + ID: clientID, ServiceUserID: "su-id", SecretHash: bcryptHash(t, "s3cret"), + }, nil) + prom.On("Sudo", mock.Anything, "su-id", schema.AdminRelationName).Return(nil) + + assert.NoError(t, ensureBootstrapSuperUser(ctx, logger, cfg, users, creds, prom)) + prom.AssertExpectations(t) + creds.AssertNotCalled(t, "Delete", mock.Anything, mock.Anything) + creds.AssertNotCalled(t, "Create", mock.Anything, mock.Anything) + users.AssertNotCalled(t, "Create", mock.Anything, mock.Anything) + }) + + t.Run("rotates the secret when it changed, then ensures superuser", func(t *testing.T) { + users, creds, prom := new(mockSUCreator), new(mockCredStore), new(mockSUPromoter) + cfg := SuperUserBootstrapConfig{ClientID: clientID, ClientSecret: "new-secret"} + + creds.On("Get", mock.Anything, clientID).Return(serviceuser.Credential{ + ID: clientID, ServiceUserID: "su-id", SecretHash: bcryptHash(t, "old-secret"), Title: "t", + }, nil) + creds.On("Delete", mock.Anything, clientID).Return(nil) + var rotated serviceuser.Credential + creds.On("Create", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { rotated = args.Get(1).(serviceuser.Credential) }). + Return(serviceuser.Credential{ID: clientID}, nil) + prom.On("Sudo", mock.Anything, "su-id", schema.AdminRelationName).Return(nil) + + assert.NoError(t, ensureBootstrapSuperUser(ctx, logger, cfg, users, creds, prom)) + creds.AssertExpectations(t) + prom.AssertExpectations(t) + users.AssertNotCalled(t, "Create", mock.Anything, mock.Anything) + + assert.Equal(t, clientID, rotated.ID) + assert.Equal(t, "su-id", rotated.ServiceUserID) + assert.NoError(t, bcrypt.CompareHashAndPassword([]byte(rotated.SecretHash), []byte("new-secret"))) + }) + + t.Run("rolls back the service user when credential creation fails", func(t *testing.T) { + users, creds, prom := new(mockSUCreator), new(mockCredStore), new(mockSUPromoter) + cfg := SuperUserBootstrapConfig{ClientID: clientID, ClientSecret: "s3cret"} + + creds.On("Get", mock.Anything, clientID).Return(serviceuser.Credential{}, serviceuser.ErrCredNotExist) + users.On("Create", mock.Anything, mock.Anything).Return(serviceuser.ServiceUser{ID: "su-id"}, nil) + creds.On("Create", mock.Anything, mock.Anything).Return(serviceuser.Credential{}, errors.New("db down")) + users.On("Delete", mock.Anything, "su-id").Return(nil) + + assert.Error(t, ensureBootstrapSuperUser(ctx, logger, cfg, users, creds, prom)) + users.AssertExpectations(t) // Create + compensating Delete both invoked + creds.AssertExpectations(t) + prom.AssertNotCalled(t, "Sudo", mock.Anything, mock.Anything, mock.Anything) + }) + + t.Run("aborts on a non-not-found credential lookup error", func(t *testing.T) { + users, creds, prom := new(mockSUCreator), new(mockCredStore), new(mockSUPromoter) + cfg := SuperUserBootstrapConfig{ClientID: clientID, ClientSecret: "s3cret"} + + creds.On("Get", mock.Anything, clientID).Return(serviceuser.Credential{}, errors.New("db down")) + + assert.Error(t, ensureBootstrapSuperUser(ctx, logger, cfg, users, creds, prom)) + users.AssertNotCalled(t, "Create", mock.Anything, mock.Anything) + prom.AssertNotCalled(t, "Sudo", mock.Anything, mock.Anything, mock.Anything) + }) +} diff --git a/internal/bootstrap/schema/schema.go b/internal/bootstrap/schema/schema.go index 21b59937e..86e35025c 100644 --- a/internal/bootstrap/schema/schema.go +++ b/internal/bootstrap/schema/schema.go @@ -17,6 +17,13 @@ const ( // Global IDs PlatformID = "platform" + // BootstrapServiceUserID is the fixed, well-known service-user id of the + // config-bootstrapped superuser SA (app.admin.bootstrap). It is deliberately + // a non-nil sentinel: uuid.Nil is reserved for the audit "system" actor, so a + // real principal must not use it. This well-known id lets the platform API + // refuse to remove the SA and the reconciler exclude it, without a lookup. + BootstrapServiceUserID = "00000000-0000-0000-0000-000000000001" + // namespace PlatformNamespace = "app/platform" OrganizationNamespace = "app/organization" diff --git a/internal/bootstrap/service.go b/internal/bootstrap/service.go index af97e16a2..0e2b10aea 100644 --- a/internal/bootstrap/service.go +++ b/internal/bootstrap/service.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "log/slog" - "strings" "github.com/raystack/frontier/billing/plan" @@ -44,10 +43,6 @@ type RelationService interface { Delete(ctx context.Context, rel relation.Relation) error } -type UserService interface { - Sudo(ctx context.Context, id string, relationName string) error -} - type FileService interface { GetDefinition(ctx context.Context) (*schema.ServiceDefinition, error) } @@ -85,9 +80,10 @@ type ServiceUserBackfiller interface { // AdminConfig is platform administration configuration type AdminConfig struct { - // Users are a list of email-ids/uuids which needs to be promoted as superusers - // if email is provided and user doesn't exist, user is created by default - Users []string `yaml:"users" mapstructure:"users"` + // Bootstrap seeds a superuser service account from config (client_id/secret) so + // automation (e.g. GitOps) has a superuser identity without a chicken-and-egg. + // All other platform-user management is handled out-of-band via GitOps reconcile. + Bootstrap SuperUserBootstrapConfig `yaml:"bootstrap" mapstructure:"bootstrap"` } type Service struct { @@ -98,12 +94,15 @@ type Service struct { roleService RoleService permissionService PermissionService authzEngine AuthzEngine - userService UserService relationService RelationService policyService PolicyService serviceuserRepo ServiceUserBackfiller patDeniedPerms map[string]struct{} + suCreator ServiceUserCreator + suCredStore ServiceUserCredentialStore + suPromoter SuperUserPromoter + planService PlanService planLocalRepo BillingPlanRepository } @@ -115,7 +114,6 @@ func NewBootstrapService( namespaceService NamespaceService, roleService RoleService, actionService PermissionService, - userService UserService, authzEngine AuthzEngine, relationService RelationService, policyService PolicyService, @@ -123,6 +121,9 @@ func NewBootstrapService( patDeniedPerms map[string]struct{}, planService PlanService, planLocalRepo BillingPlanRepository, + suCreator ServiceUserCreator, + suCredStore ServiceUserCredentialStore, + suPromoter SuperUserPromoter, ) *Service { return &Service{ logger: logger, @@ -131,7 +132,6 @@ func NewBootstrapService( namespaceService: namespaceService, roleService: roleService, permissionService: actionService, - userService: userService, authzEngine: authzEngine, planService: planService, planLocalRepo: planLocalRepo, @@ -139,6 +139,9 @@ func NewBootstrapService( policyService: policyService, serviceuserRepo: serviceuserRepo, patDeniedPerms: patDeniedPerms, + suCreator: suCreator, + suCredStore: suCredStore, + suPromoter: suPromoter, } } @@ -258,18 +261,6 @@ func filterDefaultAppNamespacePermissions(permissions []schema.ResourcePermissio return filteredPermissions } -// MakeSuperUsers promote ordinary users to superuser -func (s Service) MakeSuperUsers(ctx context.Context) error { - for _, userID := range s.adminConfig.Users { - userID = strings.TrimSpace(userID) - slog.DebugContext(ctx, "promoting user to superuser", "user_id", userID) - if err := s.userService.Sudo(ctx, userID, schema.AdminRelationName); err != nil { - return err - } - } - return nil -} - // MigrateRoles migrate predefined roles to org func (s Service) MigrateRoles(ctx context.Context) error { var err error diff --git a/internal/reconcile/platformuser.go b/internal/reconcile/platformuser.go new file mode 100644 index 000000000..6af57017d --- /dev/null +++ b/internal/reconcile/platformuser.go @@ -0,0 +1,150 @@ +package reconcile + +import ( + "fmt" + "strings" + + "github.com/raystack/frontier/internal/bootstrap/schema" +) + +// KindPlatformUser is the desired-state document kind for platform users. +const KindPlatformUser = "PlatformUser" + +const ( + principalTypeUser = "user" + principalTypeServiceUser = "serviceuser" +) + +// PlatformUserSpec is one desired platform-user entry from the YAML spec. +// Role maps directly to the platform relation (admin -> superuser, member -> check); +// the role label and the relation name are the same string ("admin" / "member"). +type PlatformUserSpec struct { + Type string `yaml:"type"` // "user" | "serviceuser" + Ref string `yaml:"ref"` // email/uuid/slug for a user; id for a service user + Role string `yaml:"role"` // "admin" | "member" +} + +// platformPrincipal is the current state of one platform principal (from ListPlatformUsers), +// with the set of platform relations it currently holds. +type platformPrincipal struct { + Type string + ID string + Email string // users only + Relations map[string]struct{} +} + +// opAction is an apply operation kind. +type opAction string + +const ( + opAdd opAction = "add" + opRemove opAction = "remove" +) + +// Op is a single planned change against one (principal, relation): add ensures the +// relation, remove strips just that relation (relation-selective). Ref is the desired +// entry's ref for add (email/id) and the current principal's id for remove. +type Op struct { + Action opAction + Type string + Ref string + Relation string +} + +func (o Op) String() string { + if o.Action == opRemove { + return fmt.Sprintf("remove %s %s (%s)", o.Type, o.Ref, o.Relation) + } + return fmt.Sprintf("add %s %s as %s", o.Type, o.Ref, o.Relation) +} + +func validateSpec(s PlatformUserSpec) error { + switch s.Type { + case principalTypeUser, principalTypeServiceUser: + default: + return fmt.Errorf("invalid type %q (want %q or %q)", s.Type, principalTypeUser, principalTypeServiceUser) + } + switch s.Role { + case schema.AdminRelationName, schema.MemberRelationName: + default: + return fmt.Errorf("invalid role %q (want %q or %q)", s.Role, schema.AdminRelationName, schema.MemberRelationName) + } + if strings.TrimSpace(s.Ref) == "" { + return fmt.Errorf("empty ref") + } + return nil +} + +// specMatchesPrincipal reports whether a desired spec refers to a current principal. +// Users match by id or email; service users by id. +func specMatchesPrincipal(s PlatformUserSpec, p platformPrincipal) bool { + if s.Type != p.Type { + return false + } + if s.Ref == p.ID { + return true + } + return p.Type == principalTypeUser && s.Ref != "" && strings.EqualFold(s.Ref, p.Email) +} + +// platformRelationOrder gives operations a stable, deterministic order. +var platformRelationOrder = []string{schema.AdminRelationName, schema.MemberRelationName} + +// diffPlatformUsers converges current platform principals to the desired spec at +// the (principal, relation) granularity: each current relation no longer desired is +// removed (relation-selectively), each desired relation not present is added. Desired +// entries matching no current principal are new and are added. Output is deterministic. +func diffPlatformUsers(desired []PlatformUserSpec, current []platformPrincipal) ([]Op, error) { + for _, s := range desired { + if err := validateSpec(s); err != nil { + return nil, fmt.Errorf("invalid platform-user spec %+v: %w", s, err) + } + } + + var ops []Op + matched := make([]bool, len(desired)) + + for _, p := range current { + want := map[string]struct{}{} + for i, s := range desired { + if specMatchesPrincipal(s, p) { + matched[i] = true + want[s.Role] = struct{}{} + } + } + // remove current relations that are no longer desired + for _, rel := range platformRelationOrder { + if _, has := p.Relations[rel]; !has { + continue + } + if _, wanted := want[rel]; !wanted { + ops = append(ops, Op{Action: opRemove, Type: p.Type, Ref: p.ID, Relation: rel}) + } + } + // add desired relations not already held + for _, rel := range platformRelationOrder { + if _, wanted := want[rel]; !wanted { + continue + } + if _, has := p.Relations[rel]; !has { + ops = append(ops, Op{Action: opAdd, Type: p.Type, Ref: p.ID, Relation: rel}) + } + } + } + + // add desired entries that match no current platform principal + seenNewRef := map[string]struct{}{} + for i, s := range desired { + if matched[i] { + continue + } + key := s.Type + "\x00" + s.Ref + "\x00" + s.Role + if _, dup := seenNewRef[key]; dup { + continue + } + seenNewRef[key] = struct{}{} + ops = append(ops, Op{Action: opAdd, Type: s.Type, Ref: s.Ref, Relation: s.Role}) + } + + return ops, nil +} diff --git a/internal/reconcile/platformuser_reconciler.go b/internal/reconcile/platformuser_reconciler.go new file mode 100644 index 000000000..6e95bec50 --- /dev/null +++ b/internal/reconcile/platformuser_reconciler.go @@ -0,0 +1,159 @@ +package reconcile + +import ( + "context" + "fmt" + "strings" + + "connectrpc.com/connect" + "github.com/raystack/frontier/internal/bootstrap/schema" + frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" + "google.golang.org/protobuf/types/known/structpb" + "gopkg.in/yaml.v3" +) + +// PlatformUserAPI is the subset of the admin API the platform-user reconciler needs. +// frontierv1beta1connect.AdminServiceClient satisfies it. +type PlatformUserAPI interface { + ListPlatformUsers(context.Context, *connect.Request[frontierv1beta1.ListPlatformUsersRequest]) (*connect.Response[frontierv1beta1.ListPlatformUsersResponse], error) + AddPlatformUser(context.Context, *connect.Request[frontierv1beta1.AddPlatformUserRequest]) (*connect.Response[frontierv1beta1.AddPlatformUserResponse], error) + RemovePlatformUser(context.Context, *connect.Request[frontierv1beta1.RemovePlatformUserRequest]) (*connect.Response[frontierv1beta1.RemovePlatformUserResponse], error) +} + +// PlatformUserReconciler converges platform admins/members to the desired spec. +type PlatformUserReconciler struct { + client PlatformUserAPI + header string // "key:value" auth header applied to each request (may be empty) +} + +func NewPlatformUserReconciler(client PlatformUserAPI, header string) *PlatformUserReconciler { + return &PlatformUserReconciler{client: client, header: header} +} + +func (r *PlatformUserReconciler) Kind() string { return KindPlatformUser } + +func (r *PlatformUserReconciler) Reconcile(ctx context.Context, spec []byte, dryRun bool) (Report, error) { + var specs []PlatformUserSpec + if err := yaml.Unmarshal(spec, &specs); err != nil { + return Report{}, fmt.Errorf("parse %s spec: %w", KindPlatformUser, err) + } + + current, err := r.fetchCurrent(ctx) + if err != nil { + return Report{}, err + } + + ops, err := diffPlatformUsers(specs, current) + if err != nil { + return Report{}, err + } + + rep := Report{Kind: KindPlatformUser, DryRun: dryRun} + for _, op := range ops { + rep.Planned = append(rep.Planned, op.String()) + } + if dryRun { + return rep, nil + } + for _, op := range ops { + if err := r.apply(ctx, op); err != nil { + return rep, fmt.Errorf("apply [%s]: %w", op, err) + } + rep.Applied++ + } + return rep, nil +} + +func (r *PlatformUserReconciler) fetchCurrent(ctx context.Context) ([]platformPrincipal, error) { + resp, err := r.client.ListPlatformUsers(ctx, authReq(&frontierv1beta1.ListPlatformUsersRequest{}, r.header)) + if err != nil { + return nil, fmt.Errorf("list platform users: %w", err) + } + var current []platformPrincipal + for _, u := range resp.Msg.GetUsers() { + current = append(current, platformPrincipal{ + Type: principalTypeUser, + ID: u.GetId(), + Email: u.GetEmail(), + Relations: relationsFromMetadata(u.GetMetadata()), + }) + } + for _, su := range resp.Msg.GetServiceusers() { + // The config-bootstrapped break-glass SA has a fixed, well-known id; it is + // server-managed (seeded at boot, removal-guarded), so never reconcile — and + // so never try to remove — it. + if su.GetId() == schema.BootstrapServiceUserID { + continue + } + current = append(current, platformPrincipal{ + Type: principalTypeServiceUser, + ID: su.GetId(), + Relations: relationsFromMetadata(su.GetMetadata()), + }) + } + return current, nil +} + +func (r *PlatformUserReconciler) apply(ctx context.Context, op Op) error { + switch op.Action { + case opAdd: + req := &frontierv1beta1.AddPlatformUserRequest{Relation: op.Relation} + if op.Type == principalTypeUser { + req.UserId = op.Ref + } else { + req.ServiceuserId = op.Ref + } + _, err := r.client.AddPlatformUser(ctx, authReq(req, r.header)) + return err + case opRemove: + // relation-selective removal (proton #489): strip only op.Relation. + req := &frontierv1beta1.RemovePlatformUserRequest{Relation: op.Relation} + if op.Type == principalTypeUser { + req.UserId = op.Ref + } else { + req.ServiceuserId = op.Ref + } + _, err := r.client.RemovePlatformUser(ctx, authReq(req, r.header)) + return err + default: + return fmt.Errorf("unknown op action %q", op.Action) + } +} + +// relationsFromMetadata reads the platform relations ListPlatformUsers stamps into +// each principal's metadata. It prefers the full "relations" list (so a principal +// holding both admin and member is reconciled exactly) and falls back to the +// single "relation" field for backward compatibility. +func relationsFromMetadata(md *structpb.Struct) map[string]struct{} { + rels := map[string]struct{}{} + if md == nil { + return rels + } + fields := md.GetFields() + if v, ok := fields["relations"]; ok { + for _, item := range v.GetListValue().GetValues() { + if name := item.GetStringValue(); name != "" { + rels[name] = struct{}{} + } + } + } + if len(rels) == 0 { + if v, ok := fields["relation"]; ok { + if name := v.GetStringValue(); name != "" { + rels[name] = struct{}{} + } + } + } + return rels +} + +// authReq builds a connect request and applies the optional "key:value" auth header. +func authReq[T any](msg *T, header string) *connect.Request[T] { + req := connect.NewRequest(msg) + if header != "" { + if k, v, ok := strings.Cut(header, ":"); ok { + req.Header().Set(strings.TrimSpace(k), strings.TrimSpace(v)) + } + } + return req +} diff --git a/internal/reconcile/platformuser_reconciler_test.go b/internal/reconcile/platformuser_reconciler_test.go new file mode 100644 index 000000000..daaceb1c4 --- /dev/null +++ b/internal/reconcile/platformuser_reconciler_test.go @@ -0,0 +1,151 @@ +package reconcile + +import ( + "context" + "testing" + + "connectrpc.com/connect" + "github.com/raystack/frontier/internal/bootstrap/schema" + frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" + "github.com/stretchr/testify/assert" + "google.golang.org/protobuf/types/known/structpb" +) + +type fakePlatformUserAPI struct { + users []*frontierv1beta1.User + sus []*frontierv1beta1.ServiceUser + added []*frontierv1beta1.AddPlatformUserRequest + removed []*frontierv1beta1.RemovePlatformUserRequest +} + +func (f *fakePlatformUserAPI) ListPlatformUsers(_ context.Context, _ *connect.Request[frontierv1beta1.ListPlatformUsersRequest]) (*connect.Response[frontierv1beta1.ListPlatformUsersResponse], error) { + return connect.NewResponse(&frontierv1beta1.ListPlatformUsersResponse{Users: f.users, Serviceusers: f.sus}), nil +} + +func (f *fakePlatformUserAPI) AddPlatformUser(_ context.Context, req *connect.Request[frontierv1beta1.AddPlatformUserRequest]) (*connect.Response[frontierv1beta1.AddPlatformUserResponse], error) { + f.added = append(f.added, req.Msg) + return connect.NewResponse(&frontierv1beta1.AddPlatformUserResponse{}), nil +} + +func (f *fakePlatformUserAPI) RemovePlatformUser(_ context.Context, req *connect.Request[frontierv1beta1.RemovePlatformUserRequest]) (*connect.Response[frontierv1beta1.RemovePlatformUserResponse], error) { + f.removed = append(f.removed, req.Msg) + return connect.NewResponse(&frontierv1beta1.RemovePlatformUserResponse{}), nil +} + +func platformUserPB(t *testing.T, id, email, relation string) *frontierv1beta1.User { + t.Helper() + md, err := structpb.NewStruct(map[string]interface{}{"relation": relation}) + if err != nil { + t.Fatalf("struct: %v", err) + } + return &frontierv1beta1.User{Id: id, Email: email, Metadata: md} +} + +// platformUserPBRelations stamps the full "relations" list the way ListPlatformUsers does. +func platformUserPBRelations(t *testing.T, id, email string, relations ...string) *frontierv1beta1.User { + t.Helper() + vals := make([]interface{}, len(relations)) + for i, r := range relations { + vals[i] = r + } + md, err := structpb.NewStruct(map[string]interface{}{"relations": vals}) + if err != nil { + t.Fatalf("struct: %v", err) + } + return &frontierv1beta1.User{Id: id, Email: email, Metadata: md} +} + +// serviceUserPBRelations builds a platform service-user list entry. +func serviceUserPBRelations(t *testing.T, id string, relations ...string) *frontierv1beta1.ServiceUser { + t.Helper() + vals := make([]interface{}, len(relations)) + for i, r := range relations { + vals[i] = r + } + md, err := structpb.NewStruct(map[string]interface{}{"relations": vals}) + if err != nil { + t.Fatalf("struct: %v", err) + } + return &frontierv1beta1.ServiceUser{Id: id, Metadata: md} +} + +func TestPlatformUserReconciler_Reconcile(t *testing.T) { + // desired: one new admin; the existing "drop" admin is absent -> removed. + specYAML := []byte("- {type: user, ref: new@x.com, role: admin}\n") + + t.Run("applies adds and removes", func(t *testing.T) { + api := &fakePlatformUserAPI{users: []*frontierv1beta1.User{ + platformUserPB(t, "drop-id", "drop@x.com", schema.AdminRelationName), + }} + rep, err := NewPlatformUserReconciler(api, "").Reconcile(context.Background(), specYAML, false) + + assert.NoError(t, err) + assert.Equal(t, 2, rep.Applied) + if assert.Len(t, api.removed, 1) { + assert.Equal(t, "drop-id", api.removed[0].GetUserId()) + assert.Equal(t, schema.AdminRelationName, api.removed[0].GetRelation()) + } + if assert.Len(t, api.added, 1) { + assert.Equal(t, "new@x.com", api.added[0].GetUserId()) + assert.Equal(t, schema.AdminRelationName, api.added[0].GetRelation()) + } + }) + + t.Run("dry-run plans without applying", func(t *testing.T) { + api := &fakePlatformUserAPI{users: []*frontierv1beta1.User{ + platformUserPB(t, "drop-id", "drop@x.com", schema.AdminRelationName), + }} + rep, err := NewPlatformUserReconciler(api, "").Reconcile(context.Background(), specYAML, true) + + assert.NoError(t, err) + assert.True(t, rep.DryRun) + assert.NotEmpty(t, rep.Planned) + assert.Equal(t, 0, rep.Applied) + assert.Empty(t, api.added) + assert.Empty(t, api.removed) + }) + + t.Run("no changes when already converged", func(t *testing.T) { + api := &fakePlatformUserAPI{users: []*frontierv1beta1.User{ + platformUserPB(t, "new-id", "new@x.com", schema.AdminRelationName), + }} + rep, err := NewPlatformUserReconciler(api, "").Reconcile(context.Background(), specYAML, false) + + assert.NoError(t, err) + assert.Empty(t, rep.Planned) + assert.Empty(t, api.added) + assert.Empty(t, api.removed) + }) + + t.Run("never removes the bootstrap service account (well-known id)", func(t *testing.T) { + // empty desired state => fully authoritative removal, but the bootstrap SA + // is matched by its well-known id and must be excluded and left untouched. + api := &fakePlatformUserAPI{sus: []*frontierv1beta1.ServiceUser{ + serviceUserPBRelations(t, schema.BootstrapServiceUserID, schema.AdminRelationName), + }} + rep, err := NewPlatformUserReconciler(api, "").Reconcile(context.Background(), []byte("[]\n"), false) + + assert.NoError(t, err) + assert.Empty(t, rep.Planned) + assert.Empty(t, api.added) + assert.Empty(t, api.removed) + }) + + t.Run("strips the extra relation when the principal holds both (relations list)", func(t *testing.T) { + // alice is desired as admin only but currently holds admin + member; + // the reconciler must read both relations and revoke member exactly. + yamlSpec := []byte("- {type: user, ref: alice@x.com, role: admin}\n") + api := &fakePlatformUserAPI{users: []*frontierv1beta1.User{ + platformUserPBRelations(t, "alice-id", "alice@x.com", schema.AdminRelationName, schema.MemberRelationName), + }} + rep, err := NewPlatformUserReconciler(api, "").Reconcile(context.Background(), yamlSpec, false) + + assert.NoError(t, err) + assert.Equal(t, 1, rep.Applied) + assert.Empty(t, api.added) + if assert.Len(t, api.removed, 1) { + assert.Equal(t, "alice-id", api.removed[0].GetUserId()) + assert.Equal(t, schema.MemberRelationName, api.removed[0].GetRelation()) + } + }) +} diff --git a/internal/reconcile/platformuser_test.go b/internal/reconcile/platformuser_test.go new file mode 100644 index 000000000..8d9ccdd6e --- /dev/null +++ b/internal/reconcile/platformuser_test.go @@ -0,0 +1,129 @@ +package reconcile + +import ( + "testing" + + "github.com/raystack/frontier/internal/bootstrap/schema" + "github.com/stretchr/testify/assert" +) + +func principal(typ, id, email string, relations ...string) platformPrincipal { + rels := make(map[string]struct{}, len(relations)) + for _, r := range relations { + rels[r] = struct{}{} + } + return platformPrincipal{Type: typ, ID: id, Email: email, Relations: rels} +} + +func TestDiffPlatformUsers(t *testing.T) { + admin := schema.AdminRelationName + member := schema.MemberRelationName + + t.Run("no desired, no current -> no ops", func(t *testing.T) { + ops, err := diffPlatformUsers(nil, nil) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + t.Run("new user is added", func(t *testing.T) { + ops, err := diffPlatformUsers( + []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Role: admin}}, + nil, + ) + assert.NoError(t, err) + assert.Equal(t, []Op{{Action: opAdd, Type: "user", Ref: "alice@x.com", Relation: admin}}, ops) + }) + + t.Run("new service user is added", func(t *testing.T) { + ops, err := diffPlatformUsers( + []PlatformUserSpec{{Type: "serviceuser", Ref: "su-1", Role: member}}, + nil, + ) + assert.NoError(t, err) + assert.Equal(t, []Op{{Action: opAdd, Type: "serviceuser", Ref: "su-1", Relation: member}}, ops) + }) + + t.Run("already-correct principal is a no-op (matched by email)", func(t *testing.T) { + ops, err := diffPlatformUsers( + []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Role: admin}}, + []platformPrincipal{principal("user", "alice-id", "alice@x.com", admin)}, + ) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + t.Run("undesired principal is removed", func(t *testing.T) { + ops, err := diffPlatformUsers( + nil, + []platformPrincipal{principal("user", "drop-id", "drop@x.com", admin)}, + ) + assert.NoError(t, err) + assert.Equal(t, []Op{{Action: opRemove, Type: "user", Ref: "drop-id", Relation: admin}}, ops) + }) + + t.Run("role change removes then re-adds the desired relation", func(t *testing.T) { + ops, err := diffPlatformUsers( + []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Role: member}}, + []platformPrincipal{principal("user", "alice-id", "alice@x.com", admin)}, + ) + assert.NoError(t, err) + assert.Equal(t, []Op{ + {Action: opRemove, Type: "user", Ref: "alice-id", Relation: admin}, + {Action: opAdd, Type: "user", Ref: "alice-id", Relation: member}, + }, ops) + }) + + t.Run("mixed: keep one, add one, remove one", func(t *testing.T) { + ops, err := diffPlatformUsers( + []PlatformUserSpec{ + {Type: "user", Ref: "keep@x.com", Role: admin}, // already correct + {Type: "user", Ref: "new@x.com", Role: member}, // new -> add + }, + []platformPrincipal{ + principal("user", "keep-id", "keep@x.com", admin), // matches keep -> no-op + principal("user", "drop-id", "drop@x.com", admin), // not desired -> remove + }, + ) + assert.NoError(t, err) + assert.Equal(t, []Op{ + {Action: opRemove, Type: "user", Ref: "drop-id", Relation: admin}, + {Action: opAdd, Type: "user", Ref: "new@x.com", Relation: member}, + }, ops) + }) + + t.Run("strips the extra relation when a principal holds both but only one is desired", func(t *testing.T) { + ops, err := diffPlatformUsers( + []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Role: admin}}, + []platformPrincipal{principal("user", "alice-id", "alice@x.com", admin, member)}, + ) + assert.NoError(t, err) + assert.Equal(t, []Op{{Action: opRemove, Type: "user", Ref: "alice-id", Relation: member}}, ops) + }) + + t.Run("keeps both relations when both are desired", func(t *testing.T) { + ops, err := diffPlatformUsers( + []PlatformUserSpec{ + {Type: "user", Ref: "alice@x.com", Role: admin}, + {Type: "user", Ref: "alice@x.com", Role: member}, + }, + []platformPrincipal{principal("user", "alice-id", "alice@x.com", admin, member)}, + ) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + t.Run("rejects an invalid role", func(t *testing.T) { + _, err := diffPlatformUsers([]PlatformUserSpec{{Type: "user", Ref: "a@x.com", Role: "owner"}}, nil) + assert.Error(t, err) + }) + + t.Run("rejects an invalid type", func(t *testing.T) { + _, err := diffPlatformUsers([]PlatformUserSpec{{Type: "group", Ref: "a@x.com", Role: admin}}, nil) + assert.Error(t, err) + }) + + t.Run("rejects an empty ref", func(t *testing.T) { + _, err := diffPlatformUsers([]PlatformUserSpec{{Type: "user", Ref: "", Role: admin}}, nil) + assert.Error(t, err) + }) +} diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go new file mode 100644 index 000000000..a0ad8a603 --- /dev/null +++ b/internal/reconcile/reconcile.go @@ -0,0 +1,70 @@ +// Package reconcile converges Frontier platform resources to a declarative +// desired-state file via the admin API. It is a small framework: each resource +// kind implements Reconciler and registers under its kind, so new kinds +// (roles, plans, products, traits, ...) plug in without changing the command +// or file format. PlatformUser is the first kind. +package reconcile + +import ( + "bytes" + "context" + "fmt" + "io" + + "gopkg.in/yaml.v3" +) + +// Reconciler converges a single resource kind from its desired-state spec. +type Reconciler interface { + Kind() string + Reconcile(ctx context.Context, spec []byte, dryRun bool) (Report, error) +} + +// Report summarises what a reconcile did, or would do when dryRun. +type Report struct { + Kind string + DryRun bool + Planned []string // human-readable operations (the plan) + Applied int // number actually applied (0 when dryRun) +} + +// document is one YAML document in a desired-state file: a kind + its spec. +type document struct { + Kind string `yaml:"kind"` + Spec yaml.Node `yaml:"spec"` +} + +// Run parses a (possibly multi-document) desired-state file and dispatches each +// document to the registered reconciler for its kind. Documents apply in file +// order; the first error aborts and returns the reports gathered so far. +func Run(ctx context.Context, registry map[string]Reconciler, data []byte, dryRun bool) ([]Report, error) { + dec := yaml.NewDecoder(bytes.NewReader(data)) + var reports []Report + for { + var doc document + err := dec.Decode(&doc) + if err == io.EOF { + break + } + if err != nil { + return reports, fmt.Errorf("parse desired-state: %w", err) + } + if doc.Kind == "" { + continue // skip empty documents + } + rec, ok := registry[doc.Kind] + if !ok { + return reports, fmt.Errorf("no reconciler registered for kind %q", doc.Kind) + } + specBytes, err := yaml.Marshal(&doc.Spec) + if err != nil { + return reports, fmt.Errorf("marshal spec for kind %q: %w", doc.Kind, err) + } + rep, err := rec.Reconcile(ctx, specBytes, dryRun) + if err != nil { + return reports, fmt.Errorf("reconcile %s: %w", doc.Kind, err) + } + reports = append(reports, rep) + } + return reports, nil +} diff --git a/internal/store/postgres/serviceuser_repository.go b/internal/store/postgres/serviceuser_repository.go index 11b501091..8a5c4e8ae 100644 --- a/internal/store/postgres/serviceuser_repository.go +++ b/internal/store/postgres/serviceuser_repository.go @@ -25,7 +25,9 @@ type ServiceUserRepository struct { type serviceUserWithContext struct { ServiceUser - OrgName string `db:"org_name"` + // OrgName is nullable: a platform-level service user (e.g. the config bootstrap + // superuser) has no real organization row, so the org_name subquery is NULL. + OrgName sql.NullString `db:"org_name"` } func NewServiceUserRepository(dbc *db.Client) *ServiceUserRepository { @@ -130,7 +132,7 @@ func (s ServiceUserRepository) Create(ctx context.Context, serviceUser serviceus AuditResource{ ID: result.OrgID, Type: auditrecord.OrganizationType, - Name: result.OrgName, + Name: result.OrgName.String, }, &AuditTarget{ ID: result.ID, @@ -307,7 +309,7 @@ func (s ServiceUserRepository) Delete(ctx context.Context, id string) error { AuditResource{ ID: result.OrgID, Type: auditrecord.OrganizationType, - Name: result.OrgName, + Name: result.OrgName.String, }, &AuditTarget{ ID: result.ID, diff --git a/test/e2e/testbench/helper.go b/test/e2e/testbench/helper.go index ae98f5ddb..1d6e279c7 100644 --- a/test/e2e/testbench/helper.go +++ b/test/e2e/testbench/helper.go @@ -3,15 +3,18 @@ package testbench import ( "context" _ "embed" + "encoding/base64" "encoding/json" "errors" "fmt" "net" "net/http" "strings" + "time" "connectrpc.com/connect" "github.com/raystack/frontier/core/authenticate/strategy" + "github.com/raystack/frontier/internal/bootstrap/schema" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" "github.com/raystack/frontier/proto/v1beta1/frontierv1beta1connect" ) @@ -30,8 +33,42 @@ var ( const ( OrgAdminEmail = "admin1-group1-org1@raystack.org" TestOTP = "123456" + + // Bootstrap superuser service-account credentials, seeded via App.Admin.Bootstrap. + // Used to grant the test admin platform superuser, mirroring the production GitOps flow. + // client_id is a credential UUID (serviceuser_credentials.id is a uuid column). + BootstrapClientID = "11111111-2222-3333-4444-555555555555" + BootstrapClientSecret = "e2e-bootstrap-secret" ) +// PromoteBootstrapAdmin authenticates as the config-bootstrapped superuser service +// account (HTTP Basic client credentials) and grants the given user the platform +// admin (superuser) relation — the same path GitOps uses in production. +func PromoteBootstrapAdmin(ctx context.Context, ad frontierv1beta1connect.AdminServiceClient, email string) error { + basic := base64.StdEncoding.EncodeToString([]byte(BootstrapClientID + ":" + BootstrapClientSecret)) + authCtx := ContextWithHeaders(ctx, map[string]string{"Authorization": "Basic " + basic}) + + // Retry while the server is still coming up (CodeUnavailable). When multiple + // suites run in one process, a prior suite's Close() SIGINTs the process, so the + // next testbench's server can take a moment to accept connections. + var lastErr error + for i := 0; i < 60; i++ { + _, err := ad.AddPlatformUser(authCtx, connect.NewRequest(&frontierv1beta1.AddPlatformUserRequest{ + UserId: email, + Relation: schema.AdminRelationName, + })) + if err == nil { + return nil + } + lastErr = err + if connect.CodeOf(err) != connect.CodeUnavailable { + return fmt.Errorf("promote bootstrap admin: %w", err) + } + time.Sleep(250 * time.Millisecond) + } + return fmt.Errorf("promote bootstrap admin (server unavailable after retries): %w", lastErr) +} + // headersKey is the context key for storing headers to be sent with ConnectRPC requests. type headersKey struct{} diff --git a/test/e2e/testbench/testbench.go b/test/e2e/testbench/testbench.go index 9f4e33446..26ddecbc4 100644 --- a/test/e2e/testbench/testbench.go +++ b/test/e2e/testbench/testbench.go @@ -1,6 +1,7 @@ package testbench import ( + "context" "errors" "fmt" "net" @@ -17,6 +18,7 @@ import ( "github.com/ory/dockertest/v3" "github.com/ory/dockertest/v3/docker" "github.com/raystack/frontier/config" + "github.com/raystack/frontier/internal/bootstrap" "github.com/raystack/frontier/internal/store/spicedb" "github.com/raystack/frontier/pkg/db" ) @@ -94,7 +96,10 @@ func Init(appConfig *config.Frontier) (*TestBench, error) { PreSharedKey: preSharedKey, Consistency: spicedb.ConsistencyLevelBestEffort.String(), } - appConfig.App.Admin.Users = []string{OrgAdminEmail} + appConfig.App.Admin.Bootstrap = bootstrap.SuperUserBootstrapConfig{ + ClientID: BootstrapClientID, + ClientSecret: BootstrapClientSecret, + } appConfig.App.Webhook.EncryptionKey = "kmm4ECoWU21K2ZoyTcYLd6w7DfhoUoap" appConfig.Billing.StripeWebhookSecrets = []string{"whsec_test_secret"} appConfig.App.Authentication.Token.Claims.AddOrgIDsClaim = true @@ -134,6 +139,12 @@ func Init(appConfig *config.Frontier) (*TestBench, error) { // let frontier start time.Sleep(time.Second * 2) + + // seed the platform superuser the production way: the config-bootstrapped service + // account grants the test admin superuser via the platform API (no boot-time human flow). + if err := PromoteBootstrapAdmin(context.Background(), te.AdminClient, OrgAdminEmail); err != nil { + return nil, err + } return te, nil } From b4551837b1c3293404874b7a97189ddf8a956b0a Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Tue, 30 Jun 2026 20:33:40 +0530 Subject: [PATCH 2/8] fix(platform): harden reconcile/bootstrap per review (require spec, reject partial config, guard sentinel ref, add-before-remove, testbench cleanup) --- internal/bootstrap/bootstrapuser.go | 14 +++++---- internal/bootstrap/bootstrapuser_test.go | 9 ++++++ internal/reconcile/platformuser.go | 40 +++++++++++++----------- internal/reconcile/platformuser_test.go | 14 +++++++-- internal/reconcile/reconcile.go | 7 +++++ internal/reconcile/reconcile_test.go | 36 +++++++++++++++++++++ test/e2e/testbench/testbench.go | 3 +- 7 files changed, 94 insertions(+), 29 deletions(-) create mode 100644 internal/reconcile/reconcile_test.go diff --git a/internal/bootstrap/bootstrapuser.go b/internal/bootstrap/bootstrapuser.go index bec875cd0..afd4c98ec 100644 --- a/internal/bootstrap/bootstrapuser.go +++ b/internal/bootstrap/bootstrapuser.go @@ -33,10 +33,6 @@ type SuperUserBootstrapConfig struct { Title string `yaml:"title" mapstructure:"title"` } -func (c SuperUserBootstrapConfig) enabled() bool { - return strings.TrimSpace(c.ClientID) != "" && c.ClientSecret != "" -} - func (c SuperUserBootstrapConfig) title() string { if t := strings.TrimSpace(c.Title); t != "" { return t @@ -86,10 +82,16 @@ func ensureBootstrapSuperUser( creds ServiceUserCredentialStore, promoter SuperUserPromoter, ) error { - if !cfg.enabled() { + clientID := strings.TrimSpace(cfg.ClientID) + // Reserve the no-op path for "fully unset". A half-configured bootstrap + // (only one of client_id/client_secret) is a misconfiguration that would + // silently skip seeding the superuser — fail fast instead. + switch { + case clientID == "" && cfg.ClientSecret == "": return nil + case clientID == "" || cfg.ClientSecret == "": + return errors.New("bootstrap superuser: client_id and client_secret must be set together") } - clientID := strings.TrimSpace(cfg.ClientID) // client_id is the service-user credential's id, which is a UUID column; a // non-UUID would otherwise fail with an opaque SQL error deep in the lookup. if _, err := uuid.Parse(clientID); err != nil { diff --git a/internal/bootstrap/bootstrapuser_test.go b/internal/bootstrap/bootstrapuser_test.go index 68d2a716b..77a3df234 100644 --- a/internal/bootstrap/bootstrapuser_test.go +++ b/internal/bootstrap/bootstrapuser_test.go @@ -78,6 +78,15 @@ func TestEnsureBootstrapSuperUser(t *testing.T) { prom.AssertNotCalled(t, "Sudo", mock.Anything, mock.Anything, mock.Anything) }) + t.Run("rejects a half-configured bootstrap (one of id/secret missing)", func(t *testing.T) { + users, creds, prom := new(mockSUCreator), new(mockCredStore), new(mockSUPromoter) + + assert.Error(t, ensureBootstrapSuperUser(ctx, logger, SuperUserBootstrapConfig{ClientID: clientID}, users, creds, prom)) + assert.Error(t, ensureBootstrapSuperUser(ctx, logger, SuperUserBootstrapConfig{ClientSecret: "s3cret"}, users, creds, prom)) + creds.AssertNotCalled(t, "Get", mock.Anything, mock.Anything) + users.AssertNotCalled(t, "Create", mock.Anything, mock.Anything) + }) + t.Run("rejects a non-uuid client_id", func(t *testing.T) { users, creds, prom := new(mockSUCreator), new(mockCredStore), new(mockSUPromoter) cfg := SuperUserBootstrapConfig{ClientID: "not-a-uuid", ClientSecret: "s3cret"} diff --git a/internal/reconcile/platformuser.go b/internal/reconcile/platformuser.go index 6af57017d..def863816 100644 --- a/internal/reconcile/platformuser.go +++ b/internal/reconcile/platformuser.go @@ -20,7 +20,7 @@ const ( // the role label and the relation name are the same string ("admin" / "member"). type PlatformUserSpec struct { Type string `yaml:"type"` // "user" | "serviceuser" - Ref string `yaml:"ref"` // email/uuid/slug for a user; id for a service user + Ref string `yaml:"ref"` // email or uuid for a user; id for a service user Role string `yaml:"role"` // "admin" | "member" } @@ -72,6 +72,12 @@ func validateSpec(s PlatformUserSpec) error { if strings.TrimSpace(s.Ref) == "" { return fmt.Errorf("empty ref") } + // The bootstrap SA is server-managed (seeded at boot, removal-guarded); it must + // not be reconciled, so reject it on the desired side too — not just skipped on + // the current side. + if s.Type == principalTypeServiceUser && strings.TrimSpace(s.Ref) == schema.BootstrapServiceUserID { + return fmt.Errorf("ref %q is the bootstrap service account, which is server-managed and cannot be reconciled", s.Ref) + } return nil } @@ -101,7 +107,11 @@ func diffPlatformUsers(desired []PlatformUserSpec, current []platformPrincipal) } } - var ops []Op + // Collect adds and removes separately so adds can be emitted first (see the + // return). Apply is sequential, so an add-before-remove order means a role + // transition (e.g. admin→member) never leaves a principal with no platform + // access if a later op fails. + var adds, removes []Op matched := make([]bool, len(desired)) for _, p := range current { @@ -112,22 +122,14 @@ func diffPlatformUsers(desired []PlatformUserSpec, current []platformPrincipal) want[s.Role] = struct{}{} } } - // remove current relations that are no longer desired - for _, rel := range platformRelationOrder { - if _, has := p.Relations[rel]; !has { - continue - } - if _, wanted := want[rel]; !wanted { - ops = append(ops, Op{Action: opRemove, Type: p.Type, Ref: p.ID, Relation: rel}) - } - } - // add desired relations not already held for _, rel := range platformRelationOrder { - if _, wanted := want[rel]; !wanted { - continue - } - if _, has := p.Relations[rel]; !has { - ops = append(ops, Op{Action: opAdd, Type: p.Type, Ref: p.ID, Relation: rel}) + _, has := p.Relations[rel] + _, wanted := want[rel] + switch { + case wanted && !has: + adds = append(adds, Op{Action: opAdd, Type: p.Type, Ref: p.ID, Relation: rel}) + case has && !wanted: + removes = append(removes, Op{Action: opRemove, Type: p.Type, Ref: p.ID, Relation: rel}) } } } @@ -143,8 +145,8 @@ func diffPlatformUsers(desired []PlatformUserSpec, current []platformPrincipal) continue } seenNewRef[key] = struct{}{} - ops = append(ops, Op{Action: opAdd, Type: s.Type, Ref: s.Ref, Relation: s.Role}) + adds = append(adds, Op{Action: opAdd, Type: s.Type, Ref: s.Ref, Relation: s.Role}) } - return ops, nil + return append(adds, removes...), nil } diff --git a/internal/reconcile/platformuser_test.go b/internal/reconcile/platformuser_test.go index 8d9ccdd6e..43c6b7c36 100644 --- a/internal/reconcile/platformuser_test.go +++ b/internal/reconcile/platformuser_test.go @@ -61,15 +61,16 @@ func TestDiffPlatformUsers(t *testing.T) { assert.Equal(t, []Op{{Action: opRemove, Type: "user", Ref: "drop-id", Relation: admin}}, ops) }) - t.Run("role change removes then re-adds the desired relation", func(t *testing.T) { + t.Run("role change adds the new relation before removing the old", func(t *testing.T) { ops, err := diffPlatformUsers( []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Role: member}}, []platformPrincipal{principal("user", "alice-id", "alice@x.com", admin)}, ) assert.NoError(t, err) + // add-before-remove so a failed apply never leaves the principal access-less. assert.Equal(t, []Op{ - {Action: opRemove, Type: "user", Ref: "alice-id", Relation: admin}, {Action: opAdd, Type: "user", Ref: "alice-id", Relation: member}, + {Action: opRemove, Type: "user", Ref: "alice-id", Relation: admin}, }, ops) }) @@ -85,9 +86,10 @@ func TestDiffPlatformUsers(t *testing.T) { }, ) assert.NoError(t, err) + // adds first, then removes (across principals). assert.Equal(t, []Op{ - {Action: opRemove, Type: "user", Ref: "drop-id", Relation: admin}, {Action: opAdd, Type: "user", Ref: "new@x.com", Relation: member}, + {Action: opRemove, Type: "user", Ref: "drop-id", Relation: admin}, }, ops) }) @@ -126,4 +128,10 @@ func TestDiffPlatformUsers(t *testing.T) { _, err := diffPlatformUsers([]PlatformUserSpec{{Type: "user", Ref: "", Role: admin}}, nil) assert.Error(t, err) }) + + t.Run("rejects the server-managed bootstrap service account", func(t *testing.T) { + _, err := diffPlatformUsers( + []PlatformUserSpec{{Type: "serviceuser", Ref: schema.BootstrapServiceUserID, Role: admin}}, nil) + assert.Error(t, err) + }) } diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go index a0ad8a603..2a6f42262 100644 --- a/internal/reconcile/reconcile.go +++ b/internal/reconcile/reconcile.go @@ -10,6 +10,7 @@ import ( "context" "fmt" "io" + "strings" "gopkg.in/yaml.v3" ) @@ -60,6 +61,12 @@ func Run(ctx context.Context, registry map[string]Reconciler, data []byte, dryRu if err != nil { return reports, fmt.Errorf("marshal spec for kind %q: %w", doc.Kind, err) } + // An absent or null spec marshals to "null"/"" — almost always a typo (e.g. + // `spce:`). Never treat that as an authoritative empty state that would strip + // every principal; an intentional empty is `spec: []`. + if s := strings.TrimSpace(string(specBytes)); s == "" || s == "null" { + return reports, fmt.Errorf("document kind %q is missing its spec", doc.Kind) + } rep, err := rec.Reconcile(ctx, specBytes, dryRun) if err != nil { return reports, fmt.Errorf("reconcile %s: %w", doc.Kind, err) diff --git a/internal/reconcile/reconcile_test.go b/internal/reconcile/reconcile_test.go new file mode 100644 index 000000000..50127344f --- /dev/null +++ b/internal/reconcile/reconcile_test.go @@ -0,0 +1,36 @@ +package reconcile + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" +) + +type fakeReconciler struct{ called int } + +func (f *fakeReconciler) Kind() string { return KindPlatformUser } +func (f *fakeReconciler) Reconcile(_ context.Context, _ []byte, _ bool) (Report, error) { + f.called++ + return Report{Kind: KindPlatformUser}, nil +} + +func TestRun_SpecHandling(t *testing.T) { + t.Run("rejects a document missing its spec (not an authoritative empty state)", func(t *testing.T) { + rec := &fakeReconciler{} + reg := map[string]Reconciler{KindPlatformUser: rec} + + _, err := Run(context.Background(), reg, []byte("kind: PlatformUser\n"), false) + assert.Error(t, err) + assert.Zero(t, rec.called) // never dispatched + }) + + t.Run("dispatches a document that has a spec", func(t *testing.T) { + rec := &fakeReconciler{} + reg := map[string]Reconciler{KindPlatformUser: rec} + + _, err := Run(context.Background(), reg, []byte("kind: PlatformUser\nspec: []\n"), false) + assert.NoError(t, err) + assert.Equal(t, 1, rec.called) + }) +} diff --git a/test/e2e/testbench/testbench.go b/test/e2e/testbench/testbench.go index 26ddecbc4..fb5561d20 100644 --- a/test/e2e/testbench/testbench.go +++ b/test/e2e/testbench/testbench.go @@ -143,7 +143,8 @@ func Init(appConfig *config.Frontier) (*TestBench, error) { // seed the platform superuser the production way: the config-bootstrapped service // account grants the test admin superuser via the platform API (no boot-time human flow). if err := PromoteBootstrapAdmin(context.Background(), te.AdminClient, OrgAdminEmail); err != nil { - return nil, err + // containers + server are already up; tear them down so we don't leak. + return nil, errors.Join(err, te.Close()) } return te, nil } From 2d67630918de5f337667ab35b24ffc02acdb42ea Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Tue, 30 Jun 2026 21:32:35 +0530 Subject: [PATCH 3/8] refactor(reconcile): de-duplicate principal-id assignment in apply via Op.principalIDs --- internal/reconcile/platformuser.go | 9 ++++++++ internal/reconcile/platformuser_reconciler.go | 22 +++++++------------ 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/internal/reconcile/platformuser.go b/internal/reconcile/platformuser.go index def863816..7e6c7e194 100644 --- a/internal/reconcile/platformuser.go +++ b/internal/reconcile/platformuser.go @@ -58,6 +58,15 @@ func (o Op) String() string { return fmt.Sprintf("add %s %s as %s", o.Type, o.Ref, o.Relation) } +// principalIDs maps the op's ref onto the request id fields: a user ref fills +// user_id, anything else fills serviceuser_id. Exactly one is non-empty. +func (o Op) principalIDs() (userID, serviceUserID string) { + if o.Type == principalTypeUser { + return o.Ref, "" + } + return "", o.Ref +} + func validateSpec(s PlatformUserSpec) error { switch s.Type { case principalTypeUser, principalTypeServiceUser: diff --git a/internal/reconcile/platformuser_reconciler.go b/internal/reconcile/platformuser_reconciler.go index 6e95bec50..298fe73aa 100644 --- a/internal/reconcile/platformuser_reconciler.go +++ b/internal/reconcile/platformuser_reconciler.go @@ -95,25 +95,19 @@ func (r *PlatformUserReconciler) fetchCurrent(ctx context.Context) ([]platformPr } func (r *PlatformUserReconciler) apply(ctx context.Context, op Op) error { + // exactly one of the two ids is set, by principal type. + userID, serviceUserID := op.principalIDs() switch op.Action { case opAdd: - req := &frontierv1beta1.AddPlatformUserRequest{Relation: op.Relation} - if op.Type == principalTypeUser { - req.UserId = op.Ref - } else { - req.ServiceuserId = op.Ref - } - _, err := r.client.AddPlatformUser(ctx, authReq(req, r.header)) + _, err := r.client.AddPlatformUser(ctx, authReq(&frontierv1beta1.AddPlatformUserRequest{ + UserId: userID, ServiceuserId: serviceUserID, Relation: op.Relation, + }, r.header)) return err case opRemove: // relation-selective removal (proton #489): strip only op.Relation. - req := &frontierv1beta1.RemovePlatformUserRequest{Relation: op.Relation} - if op.Type == principalTypeUser { - req.UserId = op.Ref - } else { - req.ServiceuserId = op.Ref - } - _, err := r.client.RemovePlatformUser(ctx, authReq(req, r.header)) + _, err := r.client.RemovePlatformUser(ctx, authReq(&frontierv1beta1.RemovePlatformUserRequest{ + UserId: userID, ServiceuserId: serviceUserID, Relation: op.Relation, + }, r.header)) return err default: return fmt.Errorf("unknown op action %q", op.Action) From 40b0997513e98e071f276ba04c0638961a534ec2 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Tue, 30 Jun 2026 21:59:08 +0530 Subject: [PATCH 4/8] fix(platform): bootstrap-SA guards return generic permission_denied, log the reason instead of leaking it --- internal/api/v1beta1connect/platform.go | 6 +++++- internal/api/v1beta1connect/platform_test.go | 2 ++ internal/api/v1beta1connect/serviceuser.go | 17 +++++++++++------ internal/api/v1beta1connect/serviceuser_test.go | 4 ++++ 4 files changed, 22 insertions(+), 7 deletions(-) diff --git a/internal/api/v1beta1connect/platform.go b/internal/api/v1beta1connect/platform.go index 724ab913d..8d061b407 100644 --- a/internal/api/v1beta1connect/platform.go +++ b/internal/api/v1beta1connect/platform.go @@ -3,6 +3,7 @@ package v1beta1connect import ( "context" "fmt" + "log/slog" "sort" "strings" @@ -59,8 +60,11 @@ func (h *ConnectHandler) RemovePlatformUser(ctx context.Context, req *connect.Re // seeded and managed at boot, not via this API, while reconcile is // authoritative over service accounts — without this guard an apply (or a // stray call) would strip its superuser access until the next restart. + // Respond with the generic permission error (don't reveal that this id is the + // protected SA); the specific reason is logged only. if req.Msg.GetServiceuserId() == schema.BootstrapServiceUserID { - return nil, connect.NewError(connect.CodePermissionDenied, fmt.Errorf("cannot remove the bootstrap superuser service account")) + slog.WarnContext(ctx, "refused removal of the bootstrap superuser service account", "service_user_id", req.Msg.GetServiceuserId()) + return nil, connect.NewError(connect.CodePermissionDenied, ErrUnauthorized) } for _, relationName := range platformRelations { if err := h.serviceUserService.UnSudo(ctx, req.Msg.GetServiceuserId(), relationName); err != nil { diff --git a/internal/api/v1beta1connect/platform_test.go b/internal/api/v1beta1connect/platform_test.go index aac42bc82..1b41536c1 100644 --- a/internal/api/v1beta1connect/platform_test.go +++ b/internal/api/v1beta1connect/platform_test.go @@ -53,6 +53,8 @@ func TestHandler_RemovePlatformUser(t *testing.T) { assert.Error(t, err) assert.Nil(t, resp) assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + // must not reveal that this id is the protected bootstrap SA. + assert.NotContains(t, err.Error(), "bootstrap") serviceUserSvc.AssertNotCalled(t, "UnSudo", mock.Anything, mock.Anything, mock.Anything) }) diff --git a/internal/api/v1beta1connect/serviceuser.go b/internal/api/v1beta1connect/serviceuser.go index e9c38d984..0d08bade4 100644 --- a/internal/api/v1beta1connect/serviceuser.go +++ b/internal/api/v1beta1connect/serviceuser.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "log/slog" "connectrpc.com/connect" "github.com/lestrrat-go/jwx/v2/jwk" @@ -173,9 +174,13 @@ func (h *ConnectHandler) CreateServiceUser(ctx context.Context, request *connect // only via app.admin.bootstrap at boot — the API must never delete it or mint // credentials/keys/tokens for it (which would create a persistent, rotation-proof // superuser backdoor). Not even platform superusers are allowed. -func errBootstrapSAImmutable(id string) error { +// +// The response is the generic permission error so it doesn't reveal that this id +// is the protected SA; the specific reason is logged only. +func errBootstrapSAImmutable(ctx context.Context, id string) error { if id == schema.BootstrapServiceUserID { - return connect.NewError(connect.CodePermissionDenied, fmt.Errorf("the bootstrap superuser service account is managed via app.admin.bootstrap and cannot be modified through the API")) + slog.WarnContext(ctx, "refused API mutation of the bootstrap superuser service account", "service_user_id", id) + return connect.NewError(connect.CodePermissionDenied, ErrUnauthorized) } return nil } @@ -185,7 +190,7 @@ func (h *ConnectHandler) DeleteServiceUser(ctx context.Context, request *connect serviceUserID := request.Msg.GetId() orgID := request.Msg.GetOrgId() - if err := errBootstrapSAImmutable(serviceUserID); err != nil { + if err := errBootstrapSAImmutable(ctx, serviceUserID); err != nil { return nil, err } @@ -214,7 +219,7 @@ func (h *ConnectHandler) CreateServiceUserJWK(ctx context.Context, request *conn serviceUserID := request.Msg.GetId() title := request.Msg.GetTitle() - if err := errBootstrapSAImmutable(serviceUserID); err != nil { + if err := errBootstrapSAImmutable(ctx, serviceUserID); err != nil { return nil, err } @@ -332,7 +337,7 @@ func (h *ConnectHandler) CreateServiceUserCredential(ctx context.Context, reques serviceUserID := request.Msg.GetId() title := request.Msg.GetTitle() - if err := errBootstrapSAImmutable(serviceUserID); err != nil { + if err := errBootstrapSAImmutable(ctx, serviceUserID); err != nil { return nil, err } @@ -388,7 +393,7 @@ func (h *ConnectHandler) CreateServiceUserToken(ctx context.Context, request *co serviceUserID := request.Msg.GetId() title := request.Msg.GetTitle() - if err := errBootstrapSAImmutable(serviceUserID); err != nil { + if err := errBootstrapSAImmutable(ctx, serviceUserID); err != nil { return nil, err } diff --git a/internal/api/v1beta1connect/serviceuser_test.go b/internal/api/v1beta1connect/serviceuser_test.go index 5daa1ca3c..858e20e66 100644 --- a/internal/api/v1beta1connect/serviceuser_test.go +++ b/internal/api/v1beta1connect/serviceuser_test.go @@ -1024,6 +1024,7 @@ func TestHandler_BootstrapSAImmutable(t *testing.T) { })) assert.Nil(t, resp) assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + assert.NotContains(t, err.Error(), "bootstrap") // must not reveal the protected SA su.AssertNotCalled(t, "Delete", mock.Anything, mock.Anything) }) @@ -1035,6 +1036,7 @@ func TestHandler_BootstrapSAImmutable(t *testing.T) { })) assert.Nil(t, resp) assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + assert.NotContains(t, err.Error(), "bootstrap") // must not reveal the protected SA su.AssertNotCalled(t, "CreateSecret", mock.Anything, mock.Anything) }) @@ -1046,6 +1048,7 @@ func TestHandler_BootstrapSAImmutable(t *testing.T) { })) assert.Nil(t, resp) assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + assert.NotContains(t, err.Error(), "bootstrap") // must not reveal the protected SA su.AssertNotCalled(t, "CreateToken", mock.Anything, mock.Anything) }) @@ -1057,6 +1060,7 @@ func TestHandler_BootstrapSAImmutable(t *testing.T) { })) assert.Nil(t, resp) assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + assert.NotContains(t, err.Error(), "bootstrap") // must not reveal the protected SA su.AssertNotCalled(t, "CreateKey", mock.Anything, mock.Anything) }) } From fa60cef819ee0a9d34487d2ff71f95918bb8cef3 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Wed, 1 Jul 2026 10:22:56 +0530 Subject: [PATCH 5/8] refactor(reconcile): rename platform-user spec field 'role' to 'relation' (relation != RBAC role) --- internal/reconcile/platformuser.go | 21 ++++++------- .../reconcile/platformuser_reconciler_test.go | 4 +-- internal/reconcile/platformuser_test.go | 30 +++++++++---------- 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/internal/reconcile/platformuser.go b/internal/reconcile/platformuser.go index 7e6c7e194..db7e4e614 100644 --- a/internal/reconcile/platformuser.go +++ b/internal/reconcile/platformuser.go @@ -16,12 +16,13 @@ const ( ) // PlatformUserSpec is one desired platform-user entry from the YAML spec. -// Role maps directly to the platform relation (admin -> superuser, member -> check); -// the role label and the relation name are the same string ("admin" / "member"). +// Relation is the platform relation to grant — admin (-> superuser) or member +// (-> check). It is deliberately "relation", not "role": "role" is a distinct +// RBAC concept in Frontier, whereas these are SpiceDB relation names. type PlatformUserSpec struct { - Type string `yaml:"type"` // "user" | "serviceuser" - Ref string `yaml:"ref"` // email or uuid for a user; id for a service user - Role string `yaml:"role"` // "admin" | "member" + Type string `yaml:"type"` // "user" | "serviceuser" + Ref string `yaml:"ref"` // email or uuid for a user; id for a service user + Relation string `yaml:"relation"` // "admin" | "member" } // platformPrincipal is the current state of one platform principal (from ListPlatformUsers), @@ -73,10 +74,10 @@ func validateSpec(s PlatformUserSpec) error { default: return fmt.Errorf("invalid type %q (want %q or %q)", s.Type, principalTypeUser, principalTypeServiceUser) } - switch s.Role { + switch s.Relation { case schema.AdminRelationName, schema.MemberRelationName: default: - return fmt.Errorf("invalid role %q (want %q or %q)", s.Role, schema.AdminRelationName, schema.MemberRelationName) + return fmt.Errorf("invalid relation %q (want %q or %q)", s.Relation, schema.AdminRelationName, schema.MemberRelationName) } if strings.TrimSpace(s.Ref) == "" { return fmt.Errorf("empty ref") @@ -128,7 +129,7 @@ func diffPlatformUsers(desired []PlatformUserSpec, current []platformPrincipal) for i, s := range desired { if specMatchesPrincipal(s, p) { matched[i] = true - want[s.Role] = struct{}{} + want[s.Relation] = struct{}{} } } for _, rel := range platformRelationOrder { @@ -149,12 +150,12 @@ func diffPlatformUsers(desired []PlatformUserSpec, current []platformPrincipal) if matched[i] { continue } - key := s.Type + "\x00" + s.Ref + "\x00" + s.Role + key := s.Type + "\x00" + s.Ref + "\x00" + s.Relation if _, dup := seenNewRef[key]; dup { continue } seenNewRef[key] = struct{}{} - adds = append(adds, Op{Action: opAdd, Type: s.Type, Ref: s.Ref, Relation: s.Role}) + adds = append(adds, Op{Action: opAdd, Type: s.Type, Ref: s.Ref, Relation: s.Relation}) } return append(adds, removes...), nil diff --git a/internal/reconcile/platformuser_reconciler_test.go b/internal/reconcile/platformuser_reconciler_test.go index daaceb1c4..951fcf800 100644 --- a/internal/reconcile/platformuser_reconciler_test.go +++ b/internal/reconcile/platformuser_reconciler_test.go @@ -71,7 +71,7 @@ func serviceUserPBRelations(t *testing.T, id string, relations ...string) *front func TestPlatformUserReconciler_Reconcile(t *testing.T) { // desired: one new admin; the existing "drop" admin is absent -> removed. - specYAML := []byte("- {type: user, ref: new@x.com, role: admin}\n") + specYAML := []byte("- {type: user, ref: new@x.com, relation: admin}\n") t.Run("applies adds and removes", func(t *testing.T) { api := &fakePlatformUserAPI{users: []*frontierv1beta1.User{ @@ -134,7 +134,7 @@ func TestPlatformUserReconciler_Reconcile(t *testing.T) { t.Run("strips the extra relation when the principal holds both (relations list)", func(t *testing.T) { // alice is desired as admin only but currently holds admin + member; // the reconciler must read both relations and revoke member exactly. - yamlSpec := []byte("- {type: user, ref: alice@x.com, role: admin}\n") + yamlSpec := []byte("- {type: user, ref: alice@x.com, relation: admin}\n") api := &fakePlatformUserAPI{users: []*frontierv1beta1.User{ platformUserPBRelations(t, "alice-id", "alice@x.com", schema.AdminRelationName, schema.MemberRelationName), }} diff --git a/internal/reconcile/platformuser_test.go b/internal/reconcile/platformuser_test.go index 43c6b7c36..94aac5136 100644 --- a/internal/reconcile/platformuser_test.go +++ b/internal/reconcile/platformuser_test.go @@ -27,7 +27,7 @@ func TestDiffPlatformUsers(t *testing.T) { t.Run("new user is added", func(t *testing.T) { ops, err := diffPlatformUsers( - []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Role: admin}}, + []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Relation: admin}}, nil, ) assert.NoError(t, err) @@ -36,7 +36,7 @@ func TestDiffPlatformUsers(t *testing.T) { t.Run("new service user is added", func(t *testing.T) { ops, err := diffPlatformUsers( - []PlatformUserSpec{{Type: "serviceuser", Ref: "su-1", Role: member}}, + []PlatformUserSpec{{Type: "serviceuser", Ref: "su-1", Relation: member}}, nil, ) assert.NoError(t, err) @@ -45,7 +45,7 @@ func TestDiffPlatformUsers(t *testing.T) { t.Run("already-correct principal is a no-op (matched by email)", func(t *testing.T) { ops, err := diffPlatformUsers( - []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Role: admin}}, + []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Relation: admin}}, []platformPrincipal{principal("user", "alice-id", "alice@x.com", admin)}, ) assert.NoError(t, err) @@ -61,9 +61,9 @@ func TestDiffPlatformUsers(t *testing.T) { assert.Equal(t, []Op{{Action: opRemove, Type: "user", Ref: "drop-id", Relation: admin}}, ops) }) - t.Run("role change adds the new relation before removing the old", func(t *testing.T) { + t.Run("relation change adds the new relation before removing the old", func(t *testing.T) { ops, err := diffPlatformUsers( - []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Role: member}}, + []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Relation: member}}, []platformPrincipal{principal("user", "alice-id", "alice@x.com", admin)}, ) assert.NoError(t, err) @@ -77,8 +77,8 @@ func TestDiffPlatformUsers(t *testing.T) { t.Run("mixed: keep one, add one, remove one", func(t *testing.T) { ops, err := diffPlatformUsers( []PlatformUserSpec{ - {Type: "user", Ref: "keep@x.com", Role: admin}, // already correct - {Type: "user", Ref: "new@x.com", Role: member}, // new -> add + {Type: "user", Ref: "keep@x.com", Relation: admin}, // already correct + {Type: "user", Ref: "new@x.com", Relation: member}, // new -> add }, []platformPrincipal{ principal("user", "keep-id", "keep@x.com", admin), // matches keep -> no-op @@ -95,7 +95,7 @@ func TestDiffPlatformUsers(t *testing.T) { t.Run("strips the extra relation when a principal holds both but only one is desired", func(t *testing.T) { ops, err := diffPlatformUsers( - []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Role: admin}}, + []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Relation: admin}}, []platformPrincipal{principal("user", "alice-id", "alice@x.com", admin, member)}, ) assert.NoError(t, err) @@ -105,8 +105,8 @@ func TestDiffPlatformUsers(t *testing.T) { t.Run("keeps both relations when both are desired", func(t *testing.T) { ops, err := diffPlatformUsers( []PlatformUserSpec{ - {Type: "user", Ref: "alice@x.com", Role: admin}, - {Type: "user", Ref: "alice@x.com", Role: member}, + {Type: "user", Ref: "alice@x.com", Relation: admin}, + {Type: "user", Ref: "alice@x.com", Relation: member}, }, []platformPrincipal{principal("user", "alice-id", "alice@x.com", admin, member)}, ) @@ -114,24 +114,24 @@ func TestDiffPlatformUsers(t *testing.T) { assert.Empty(t, ops) }) - t.Run("rejects an invalid role", func(t *testing.T) { - _, err := diffPlatformUsers([]PlatformUserSpec{{Type: "user", Ref: "a@x.com", Role: "owner"}}, nil) + t.Run("rejects an invalid relation", func(t *testing.T) { + _, err := diffPlatformUsers([]PlatformUserSpec{{Type: "user", Ref: "a@x.com", Relation: "owner"}}, nil) assert.Error(t, err) }) t.Run("rejects an invalid type", func(t *testing.T) { - _, err := diffPlatformUsers([]PlatformUserSpec{{Type: "group", Ref: "a@x.com", Role: admin}}, nil) + _, err := diffPlatformUsers([]PlatformUserSpec{{Type: "group", Ref: "a@x.com", Relation: admin}}, nil) assert.Error(t, err) }) t.Run("rejects an empty ref", func(t *testing.T) { - _, err := diffPlatformUsers([]PlatformUserSpec{{Type: "user", Ref: "", Role: admin}}, nil) + _, err := diffPlatformUsers([]PlatformUserSpec{{Type: "user", Ref: "", Relation: admin}}, nil) assert.Error(t, err) }) t.Run("rejects the server-managed bootstrap service account", func(t *testing.T) { _, err := diffPlatformUsers( - []PlatformUserSpec{{Type: "serviceuser", Ref: schema.BootstrapServiceUserID, Role: admin}}, nil) + []PlatformUserSpec{{Type: "serviceuser", Ref: schema.BootstrapServiceUserID, Relation: admin}}, nil) assert.Error(t, err) }) } From 02884e4107f44c59c05ab4682138ebebfc414d9d Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Wed, 1 Jul 2026 10:38:54 +0530 Subject: [PATCH 6/8] test(reconcile): cover ref matching by user id, user email, and serviceuser id --- internal/reconcile/platformuser_test.go | 45 +++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/internal/reconcile/platformuser_test.go b/internal/reconcile/platformuser_test.go index 94aac5136..bbac1421e 100644 --- a/internal/reconcile/platformuser_test.go +++ b/internal/reconcile/platformuser_test.go @@ -135,3 +135,48 @@ func TestDiffPlatformUsers(t *testing.T) { assert.Error(t, err) }) } + +// TestSpecRefMatching covers the accepted `ref` forms: a user may be referenced by +// id or by email (case-insensitive); a service user only by id. +func TestSpecRefMatching(t *testing.T) { + admin := schema.AdminRelationName + userP := principal("user", "user-uuid-1", "alice@x.com") + suP := principal("serviceuser", "su-uuid-1", "") // service users carry no email + + t.Run("user matches by id", func(t *testing.T) { + assert.True(t, specMatchesPrincipal(PlatformUserSpec{Type: "user", Ref: "user-uuid-1", Relation: admin}, userP)) + }) + t.Run("user matches by email (case-insensitive)", func(t *testing.T) { + assert.True(t, specMatchesPrincipal(PlatformUserSpec{Type: "user", Ref: "Alice@X.com", Relation: admin}, userP)) + }) + t.Run("service user matches by id", func(t *testing.T) { + assert.True(t, specMatchesPrincipal(PlatformUserSpec{Type: "serviceuser", Ref: "su-uuid-1", Relation: admin}, suP)) + }) + t.Run("service user does NOT match by email (email is user-only)", func(t *testing.T) { + suWithEmail := principal("serviceuser", "su-uuid-1", "svc@x.com") + assert.False(t, specMatchesPrincipal(PlatformUserSpec{Type: "serviceuser", Ref: "svc@x.com", Relation: admin}, suWithEmail)) + }) + t.Run("no match when the type differs", func(t *testing.T) { + assert.False(t, specMatchesPrincipal(PlatformUserSpec{Type: "serviceuser", Ref: "user-uuid-1", Relation: admin}, userP)) + }) + t.Run("no match on an unrelated ref", func(t *testing.T) { + assert.False(t, specMatchesPrincipal(PlatformUserSpec{Type: "user", Ref: "bob@x.com", Relation: admin}, userP)) + }) + + // end-to-end through the diff: a user referenced by id and a service user by id + // both converge to no-ops when already in the desired relation. + t.Run("diff converges: user by id + serviceuser by id already correct", func(t *testing.T) { + ops, err := diffPlatformUsers( + []PlatformUserSpec{ + {Type: "user", Ref: "user-uuid-1", Relation: admin}, + {Type: "serviceuser", Ref: "su-uuid-1", Relation: admin}, + }, + []platformPrincipal{ + principal("user", "user-uuid-1", "alice@x.com", admin), + principal("serviceuser", "su-uuid-1", "", admin), + }, + ) + assert.NoError(t, err) + assert.Empty(t, ops) + }) +} From 84b818db4569dd4d07f7a550fc1290c2be7bd701 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Wed, 1 Jul 2026 16:49:03 +0530 Subject: [PATCH 7/8] fix(platform): guard bootstrap-SA credential/token/key deletes, fix reconcile applied count, tidy config + comment --- cmd/reconcile.go | 6 ++-- config/sample.config.yaml | 3 +- core/serviceuser/errors.go | 3 ++ core/serviceuser/service.go | 8 +++++ core/serviceuser/service_test.go | 28 +++++++++++++++ internal/api/v1beta1connect/platform.go | 9 ++--- internal/api/v1beta1connect/serviceuser.go | 8 +++++ .../api/v1beta1connect/serviceuser_test.go | 34 +++++++++++++++++++ 8 files changed, 89 insertions(+), 10 deletions(-) diff --git a/cmd/reconcile.go b/cmd/reconcile.go index a4ddc8566..99849f436 100644 --- a/cmd/reconcile.go +++ b/cmd/reconcile.go @@ -65,11 +65,11 @@ func printReconcileReport(cmd *cli.Command, rep reconcile.Report) { cmd.Printf("%s: no changes\n", rep.Kind) return } - verb := "applied" + verb, count := "applied", rep.Applied if rep.DryRun { - verb = "planned" + verb, count = "planned", len(rep.Planned) } - cmd.Printf("%s (%s %d):\n", rep.Kind, verb, len(rep.Planned)) + cmd.Printf("%s (%s %d):\n", rep.Kind, verb, count) for _, p := range rep.Planned { cmd.Printf(" - %s\n", p) } diff --git a/config/sample.config.yaml b/config/sample.config.yaml index f29974a61..ca124fac4 100644 --- a/config/sample.config.yaml +++ b/config/sample.config.yaml @@ -184,7 +184,8 @@ app: # guaranteed superuser identity without a chicken-and-egg. The account is ensured and # promoted to superuser on every boot (idempotent); the secret is rotated if it changes # here. Authenticate with: Authorization: Basic base64(client_id:client_secret). - # Leave client_id/client_secret empty to disable. + # Leave client_id/client_secret empty to disable — but then no superuser is seeded and, + # since granting one is itself superuser-gated, none can be created via the API either. # client_id must be a UUID (it is the service-account credential id); generate one # (e.g. uuidgen) and keep it stable. client_secret is your chosen password. bootstrap: diff --git a/core/serviceuser/errors.go b/core/serviceuser/errors.go index 2ad4336a7..ca463ec51 100644 --- a/core/serviceuser/errors.go +++ b/core/serviceuser/errors.go @@ -11,4 +11,7 @@ var ( ErrConflict = errors.New("service user already exist") ErrEmptyKey = errors.New("empty key") ErrDisabled = errors.New("service user is disabled") + // ErrProtected marks a server-managed service user (the config-bootstrapped + // bootstrap SA) whose credentials must not be mutated via the API. + ErrProtected = errors.New("service user is protected") ) diff --git a/core/serviceuser/service.go b/core/serviceuser/service.go index fb05f1e71..9b7af9e3f 100644 --- a/core/serviceuser/service.go +++ b/core/serviceuser/service.go @@ -254,6 +254,14 @@ func (s Service) GetKey(ctx context.Context, credID string) (Credential, error) } func (s Service) DeleteKey(ctx context.Context, credID string) error { + // Never let a credential/key/token of the config-bootstrapped bootstrap SA be + // deleted via the API — it is server-managed (rotated at boot). Boot rotation + // deletes via the repository directly, so it is unaffected by this guard. + if cred, err := s.credRepo.Get(ctx, credID); err == nil && cred.ServiceUserID == schema.BootstrapServiceUserID { + s.log.WarnContext(ctx, "refused deleting a credential of the bootstrap superuser service account", + "serviceuser_id", cred.ServiceUserID, "credential_id", credID) + return ErrProtected + } return s.credRepo.Delete(ctx, credID) } diff --git a/core/serviceuser/service_test.go b/core/serviceuser/service_test.go index c4ba9ac33..46c9b23c2 100644 --- a/core/serviceuser/service_test.go +++ b/core/serviceuser/service_test.go @@ -345,3 +345,31 @@ func TestService_ListByOrg(t *testing.T) { }) } } + +func TestService_DeleteKey_BootstrapProtected(t *testing.T) { + ctx := context.Background() + const credID = "cred-1" + + t.Run("refuses deleting a credential owned by the bootstrap SA", func(t *testing.T) { + svc, _, credRepo, _, _, _ := newTestService(t) + credRepo.On("Get", ctx, credID).Return( + serviceuser.Credential{ID: credID, ServiceUserID: schema.BootstrapServiceUserID}, nil) + + err := svc.DeleteSecret(ctx, credID) // funnels through DeleteKey + if !errors.Is(err, serviceuser.ErrProtected) { + t.Fatalf("want ErrProtected, got %v", err) + } + credRepo.AssertNotCalled(t, "Delete", mock.Anything, mock.Anything) + }) + + t.Run("deletes a credential of a normal service user", func(t *testing.T) { + svc, _, credRepo, _, _, _ := newTestService(t) + credRepo.On("Get", ctx, credID).Return( + serviceuser.Credential{ID: credID, ServiceUserID: "11111111-2222-3333-4444-555555555555"}, nil) + credRepo.On("Delete", ctx, credID).Return(nil) + + if err := svc.DeleteToken(ctx, credID); err != nil { // also funnels through DeleteKey + t.Fatalf("unexpected error: %v", err) + } + }) +} diff --git a/internal/api/v1beta1connect/platform.go b/internal/api/v1beta1connect/platform.go index 8d061b407..0bafc86be 100644 --- a/internal/api/v1beta1connect/platform.go +++ b/internal/api/v1beta1connect/platform.go @@ -56,12 +56,9 @@ func (h *ConnectHandler) RemovePlatformUser(ctx context.Context, req *connect.Re } } } else if req.Msg.GetServiceuserId() != "" { - // Protect the config-bootstrapped break-glass SA (well-known id). It is - // seeded and managed at boot, not via this API, while reconcile is - // authoritative over service accounts — without this guard an apply (or a - // stray call) would strip its superuser access until the next restart. - // Respond with the generic permission error (don't reveal that this id is the - // protected SA); the specific reason is logged only. + // The bootstrap SA is server-managed (seeded at boot), so it can't be removed + // via this API. Return the generic permission error and log the reason — + // the response must not reveal that this id is the protected SA. if req.Msg.GetServiceuserId() == schema.BootstrapServiceUserID { slog.WarnContext(ctx, "refused removal of the bootstrap superuser service account", "service_user_id", req.Msg.GetServiceuserId()) return nil, connect.NewError(connect.CodePermissionDenied, ErrUnauthorized) diff --git a/internal/api/v1beta1connect/serviceuser.go b/internal/api/v1beta1connect/serviceuser.go index 0d08bade4..f85f49022 100644 --- a/internal/api/v1beta1connect/serviceuser.go +++ b/internal/api/v1beta1connect/serviceuser.go @@ -325,6 +325,8 @@ func (h *ConnectHandler) DeleteServiceUserJWK(ctx context.Context, request *conn switch { case err == serviceuser.ErrCredNotExist: return nil, connect.NewError(connect.CodeNotFound, ErrServiceUserCredNotFound) + case errors.Is(err, serviceuser.ErrProtected): + return nil, connect.NewError(connect.CodePermissionDenied, ErrUnauthorized) default: return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("DeleteServiceUserJWK: key_id=%s: %w", keyID, err)) } @@ -384,6 +386,9 @@ func (h *ConnectHandler) DeleteServiceUserCredential(ctx context.Context, reques err := h.serviceUserService.DeleteSecret(ctx, secretID) if err != nil { + if errors.Is(err, serviceuser.ErrProtected) { + return nil, connect.NewError(connect.CodePermissionDenied, ErrUnauthorized) + } return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("DeleteServiceUserCredential: secret_id=%s: %w", secretID, err)) } return connect.NewResponse(&frontierv1beta1.DeleteServiceUserCredentialResponse{}), nil @@ -439,6 +444,9 @@ func (h *ConnectHandler) DeleteServiceUserToken(ctx context.Context, request *co err := h.serviceUserService.DeleteToken(ctx, tokenID) if err != nil { + if errors.Is(err, serviceuser.ErrProtected) { + return nil, connect.NewError(connect.CodePermissionDenied, ErrUnauthorized) + } return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("DeleteServiceUserToken: token_id=%s: %w", tokenID, err)) } return connect.NewResponse(&frontierv1beta1.DeleteServiceUserTokenResponse{}), nil diff --git a/internal/api/v1beta1connect/serviceuser_test.go b/internal/api/v1beta1connect/serviceuser_test.go index 858e20e66..e15d26e41 100644 --- a/internal/api/v1beta1connect/serviceuser_test.go +++ b/internal/api/v1beta1connect/serviceuser_test.go @@ -1063,6 +1063,40 @@ func TestHandler_BootstrapSAImmutable(t *testing.T) { assert.NotContains(t, err.Error(), "bootstrap") // must not reveal the protected SA su.AssertNotCalled(t, "CreateKey", mock.Anything, mock.Anything) }) + + // The credential/token/JWK deletes are keyed by secret id, so the service returns + // ErrProtected when the owner is the bootstrap SA; the handler must map it to a + // generic permission_denied without leaking that the SA is protected. + t.Run("DeleteServiceUserCredential is refused (generic) when service reports protected", func(t *testing.T) { + su := new(mocks.ServiceUserService) + su.On("DeleteSecret", mock.Anything, "cred-x").Return(serviceuser.ErrProtected) + h := &ConnectHandler{serviceUserService: su} + resp, err := h.DeleteServiceUserCredential(context.Background(), connect.NewRequest(&frontierv1beta1.DeleteServiceUserCredentialRequest{SecretId: "cred-x"})) + assert.Nil(t, resp) + assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + assert.NotContains(t, err.Error(), "bootstrap") + assert.NotContains(t, err.Error(), "protected") + }) + + t.Run("DeleteServiceUserToken is refused (generic) when service reports protected", func(t *testing.T) { + su := new(mocks.ServiceUserService) + su.On("DeleteToken", mock.Anything, "tok-x").Return(serviceuser.ErrProtected) + h := &ConnectHandler{serviceUserService: su} + resp, err := h.DeleteServiceUserToken(context.Background(), connect.NewRequest(&frontierv1beta1.DeleteServiceUserTokenRequest{TokenId: "tok-x"})) + assert.Nil(t, resp) + assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + assert.NotContains(t, err.Error(), "bootstrap") + }) + + t.Run("DeleteServiceUserJWK is refused (generic) when service reports protected", func(t *testing.T) { + su := new(mocks.ServiceUserService) + su.On("DeleteKey", mock.Anything, "key-x").Return(serviceuser.ErrProtected) + h := &ConnectHandler{serviceUserService: su} + resp, err := h.DeleteServiceUserJWK(context.Background(), connect.NewRequest(&frontierv1beta1.DeleteServiceUserJWKRequest{KeyId: "key-x"})) + assert.Nil(t, resp) + assert.Equal(t, connect.CodePermissionDenied, connect.CodeOf(err)) + assert.NotContains(t, err.Error(), "bootstrap") + }) } func TestHandler_DeleteServiceUserCredential(t *testing.T) { From 528cd5733be63880770d60f5ba699eaf194d2db4 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Wed, 1 Jul 2026 18:00:07 +0530 Subject: [PATCH 8/8] docs(platform): rewrite gitops reconcile and bootstrap comments in plain english --- cmd/reconcile.go | 10 ++-- config/sample.config.yaml | 17 +++---- internal/api/v1beta1connect/platform.go | 6 +-- internal/api/v1beta1connect/serviceuser.go | 14 +++--- internal/bootstrap/bootstrapuser.go | 48 ++++++++++--------- internal/bootstrap/bootstrapuser_test.go | 2 +- internal/bootstrap/schema/schema.go | 10 ++-- internal/bootstrap/service.go | 5 +- internal/reconcile/platformuser.go | 33 ++++++------- internal/reconcile/platformuser_reconciler.go | 16 +++---- .../reconcile/platformuser_reconciler_test.go | 8 ++-- internal/reconcile/platformuser_test.go | 10 ++-- internal/reconcile/reconcile.go | 24 +++++----- internal/reconcile/reconcile_test.go | 2 +- 14 files changed, 105 insertions(+), 100 deletions(-) diff --git a/cmd/reconcile.go b/cmd/reconcile.go index 99849f436..49c49348a 100644 --- a/cmd/reconcile.go +++ b/cmd/reconcile.go @@ -17,13 +17,13 @@ func ReconcileCommand(cliConfig *Config) *cli.Command { ) cmd := &cli.Command{ Use: "reconcile", - Short: "Reconcile declarative platform configuration to a desired-state file", + Short: "Reconcile platform configuration to a desired-state file", Long: heredoc.Doc(` - Converge platform resources to a declarative YAML spec via the admin API. + Make platform resources match a desired-state YAML file, through the admin API. - Currently supports the PlatformUser kind (platform admins/members). The file - is the source of truth: entries present are ensured, entries absent are removed. - Authenticate as a superuser (e.g. the bootstrap service account) via --header. + Supports the PlatformUser kind (platform admins and members) for now. The file + decides who has access: anyone listed is added, anyone not listed is removed. + Log in as a superuser (for example the bootstrap service account) with --header. `), Example: heredoc.Doc(` $ frontier reconcile -f platform-users.yaml --dry-run -H "Authorization:Basic " diff --git a/config/sample.config.yaml b/config/sample.config.yaml index ca124fac4..043451f8e 100644 --- a/config/sample.config.yaml +++ b/config/sample.config.yaml @@ -180,14 +180,15 @@ app: # platform level administration admin: # bootstrap seeds a superuser SERVICE ACCOUNT from config (a username/password-style - # client_id + client_secret) so automation like the GitOps reconcile flow has a - # guaranteed superuser identity without a chicken-and-egg. The account is ensured and - # promoted to superuser on every boot (idempotent); the secret is rotated if it changes - # here. Authenticate with: Authorization: Basic base64(client_id:client_secret). - # Leave client_id/client_secret empty to disable — but then no superuser is seeded and, - # since granting one is itself superuser-gated, none can be created via the API either. - # client_id must be a UUID (it is the service-account credential id); generate one - # (e.g. uuidgen) and keep it stable. client_secret is your chosen password. + # client_id + client_secret), so automation like the GitOps reconcile flow always has + # a superuser to log in as. This means you don't need an existing superuser to create + # the first one. The account is created and promoted to superuser on every boot (safe + # to run again), and the secret is rotated if you change it here. Log in with: + # Authorization: Basic base64(client_id:client_secret). + # Leave client_id/client_secret empty to turn this off — but then no superuser is + # seeded, and since granting one needs a superuser, none can be created via the API + # either. client_id must be a UUID (it is the service-account credential id); generate + # one (e.g. uuidgen) and keep it stable. client_secret is your chosen password. bootstrap: client_id: "" client_secret: "" diff --git a/internal/api/v1beta1connect/platform.go b/internal/api/v1beta1connect/platform.go index 791712972..bdba5e90f 100644 --- a/internal/api/v1beta1connect/platform.go +++ b/internal/api/v1beta1connect/platform.go @@ -57,9 +57,9 @@ func (h *ConnectHandler) RemovePlatformUser(ctx context.Context, req *connect.Re } } } else if req.Msg.GetServiceuserId() != "" { - // The bootstrap SA is server-managed (seeded at boot), so it can't be removed - // via this API. Return the generic permission error and log the reason — - // the response must not reveal that this id is the protected SA. + // The server manages the bootstrap SA (it seeds it at boot), so it can't be + // removed through this API. Return the generic permission error and log why. + // The response must not reveal that this id is the protected SA. if req.Msg.GetServiceuserId() == schema.BootstrapServiceUserID { slog.WarnContext(ctx, "refused removal of the bootstrap superuser service account", "service_user_id", req.Msg.GetServiceuserId()) return nil, connect.NewError(connect.CodePermissionDenied, ErrUnauthorized) diff --git a/internal/api/v1beta1connect/serviceuser.go b/internal/api/v1beta1connect/serviceuser.go index f85f49022..3731239a5 100644 --- a/internal/api/v1beta1connect/serviceuser.go +++ b/internal/api/v1beta1connect/serviceuser.go @@ -169,14 +169,14 @@ func (h *ConnectHandler) CreateServiceUser(ctx context.Context, request *connect }), nil } -// errBootstrapSAImmutable returns a PermissionDenied error when id is the -// config-bootstrapped break-glass SA (well-known id). That account is managed -// only via app.admin.bootstrap at boot — the API must never delete it or mint -// credentials/keys/tokens for it (which would create a persistent, rotation-proof -// superuser backdoor). Not even platform superusers are allowed. +// errBootstrapSAImmutable returns a PermissionDenied error when id is the bootstrap +// SA (its fixed id). That account is managed only through app.admin.bootstrap at +// boot. The API must never delete it or create credentials, keys, or tokens for it. +// Doing so would leave a superuser way in that config can't rotate away. Not even +// platform superusers are allowed. // -// The response is the generic permission error so it doesn't reveal that this id -// is the protected SA; the specific reason is logged only. +// The response is the generic permission error, so it doesn't reveal that this id is +// the protected SA. The specific reason is only logged. func errBootstrapSAImmutable(ctx context.Context, id string) error { if id == schema.BootstrapServiceUserID { slog.WarnContext(ctx, "refused API mutation of the bootstrap superuser service account", "service_user_id", id) diff --git a/internal/bootstrap/bootstrapuser.go b/internal/bootstrap/bootstrapuser.go index afd4c98ec..5a34823d5 100644 --- a/internal/bootstrap/bootstrapuser.go +++ b/internal/bootstrap/bootstrapuser.go @@ -21,12 +21,12 @@ var bootstrapBcryptCost = 14 const defaultBootstrapTitle = "GitOps Bootstrap Superuser" -// SuperUserBootstrapConfig configures a config-seeded superuser service account. -// It supplies a username/password-style credential (client_id + client_secret) -// so automation (e.g. the GitOps reconcile flow) has a guaranteed superuser -// identity at boot — removing the chicken-and-egg of needing a superuser to -// create the first superuser. The secret is operator-provided and rotated when -// it changes. Leave client_id/client_secret empty to disable. +// SuperUserBootstrapConfig configures a superuser service account seeded from +// config. It gives a username/password-style credential (client_id + client_secret) +// so automation (like the GitOps reconcile flow) always has a superuser to log in as +// at boot. This means you don't need an existing superuser to create the first one. +// You provide the secret, and it is rotated when it changes. Leave client_id and +// client_secret empty to turn this off. type SuperUserBootstrapConfig struct { ClientID string `yaml:"client_id" mapstructure:"client_id"` ClientSecret string `yaml:"client_secret" mapstructure:"client_secret"` @@ -45,9 +45,9 @@ func (c SuperUserBootstrapConfig) title() string { // is the repository's Create, not serviceuser.Service.Create. type ServiceUserCreator interface { Create(ctx context.Context, su serviceuser.ServiceUser) (serviceuser.ServiceUser, error) - // Delete removes a service user by id. Used to roll back a just-created - // bootstrap service user when credential creation fails, so repeated boot - // failures don't accumulate credential-less orphan rows. + // Delete removes a service user by id. Used to undo a just-created bootstrap + // service user when creating its credential fails, so repeated boot failures + // don't leave behind service-user rows with no credential. Delete(ctx context.Context, id string) error } @@ -63,17 +63,19 @@ type SuperUserPromoter interface { Sudo(ctx context.Context, id, relationName string) error } -// EnsureBootstrapSuperUser idempotently ensures the configured superuser service -// account exists and is a platform superuser. No-op when not configured. +// EnsureBootstrapSuperUser makes sure the configured superuser service account +// exists and is a platform superuser. It is safe to run on every boot. It does +// nothing when the account is not configured. func (s Service) EnsureBootstrapSuperUser(ctx context.Context) error { return ensureBootstrapSuperUser(ctx, s.logger, s.adminConfig.Bootstrap, s.suCreator, s.suCredStore, s.suPromoter) } // ensureBootstrapSuperUser holds the testable core. The account lives in the // platform/nil org (serviceusers.org_id is nullable, no FK) and is created -// without org membership. Idempotency is keyed on the credential id (client_id): -// - absent: create the service user + client-secret credential + promote; -// - present: rotate the secret if it changed, then (re)ensure the superuser relation. +// without org membership. It decides what to do based on whether the credential +// id (client_id) already exists: +// - not there: create the service user + client-secret credential + promote it; +// - there: rotate the secret if it changed, then make sure the superuser relation is set. func ensureBootstrapSuperUser( ctx context.Context, logger *slog.Logger, @@ -83,9 +85,9 @@ func ensureBootstrapSuperUser( promoter SuperUserPromoter, ) error { clientID := strings.TrimSpace(cfg.ClientID) - // Reserve the no-op path for "fully unset". A half-configured bootstrap - // (only one of client_id/client_secret) is a misconfiguration that would - // silently skip seeding the superuser — fail fast instead. + // Only skip when the config is fully unset. A half-configured bootstrap (only + // one of client_id/client_secret) is a mistake that would quietly skip seeding + // the superuser — fail early instead. switch { case clientID == "" && cfg.ClientSecret == "": return nil @@ -125,7 +127,7 @@ func createBootstrapSuperUser( promoter SuperUserPromoter, ) error { su, err := users.Create(ctx, serviceuser.ServiceUser{ - ID: schema.BootstrapServiceUserID, // fixed, well-known id (see schema.BootstrapServiceUserID) + ID: schema.BootstrapServiceUserID, // fixed id (see schema.BootstrapServiceUserID) OrgID: schema.PlatformOrgID.String(), Title: cfg.title(), }) @@ -152,11 +154,11 @@ func createBootstrapSuperUser( return promoteBootstrapSuperUser(ctx, promoter, su.ID) } -// cleanupOrphanBootstrapSU best-effort deletes a service user created moments -// earlier when a later step (hashing, credential creation) fails. Without it the -// row would be orphaned — credential-less and unpromoted — and the next boot, -// still finding no credential, would create another. The original cause is always -// returned; a failed cleanup is only logged (the next boot retries anyway). +// cleanupOrphanBootstrapSU tries to delete a service user that was just created +// when a later step (hashing, or creating the credential) fails. Without it the +// row would be left behind with no credential and no superuser grant, and the next +// boot, still finding no credential, would create another one. The original error +// is always returned; a failed cleanup is only logged (the next boot retries anyway). func cleanupOrphanBootstrapSU(ctx context.Context, logger *slog.Logger, users ServiceUserCreator, suID string, cause error) error { if err := users.Delete(ctx, suID); err != nil { logger.WarnContext(ctx, "failed to roll back orphan bootstrap service user", diff --git a/internal/bootstrap/bootstrapuser_test.go b/internal/bootstrap/bootstrapuser_test.go index 77a3df234..114f75835 100644 --- a/internal/bootstrap/bootstrapuser_test.go +++ b/internal/bootstrap/bootstrapuser_test.go @@ -69,7 +69,7 @@ func TestEnsureBootstrapSuperUser(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, nil)) const clientID = "f47ac10b-58cc-4372-a567-0e02b2c3d479" - t.Run("no-op when not configured", func(t *testing.T) { + t.Run("does nothing when not configured", func(t *testing.T) { users, creds, prom := new(mockSUCreator), new(mockCredStore), new(mockSUPromoter) assert.NoError(t, ensureBootstrapSuperUser(ctx, logger, SuperUserBootstrapConfig{}, users, creds, prom)) diff --git a/internal/bootstrap/schema/schema.go b/internal/bootstrap/schema/schema.go index 86e35025c..0b671c377 100644 --- a/internal/bootstrap/schema/schema.go +++ b/internal/bootstrap/schema/schema.go @@ -17,11 +17,11 @@ const ( // Global IDs PlatformID = "platform" - // BootstrapServiceUserID is the fixed, well-known service-user id of the - // config-bootstrapped superuser SA (app.admin.bootstrap). It is deliberately - // a non-nil sentinel: uuid.Nil is reserved for the audit "system" actor, so a - // real principal must not use it. This well-known id lets the platform API - // refuse to remove the SA and the reconciler exclude it, without a lookup. + // BootstrapServiceUserID is the fixed service-user id of the bootstrap superuser + // SA from config (app.admin.bootstrap). It is deliberately not uuid.Nil: uuid.Nil + // is reserved for the audit "system" actor, so a real principal must not use it. + // Because this id is fixed, the platform API can refuse to remove the SA and the + // reconciler can skip it, with no lookup needed. BootstrapServiceUserID = "00000000-0000-0000-0000-000000000001" // namespace diff --git a/internal/bootstrap/service.go b/internal/bootstrap/service.go index 0e2b10aea..12407a65d 100644 --- a/internal/bootstrap/service.go +++ b/internal/bootstrap/service.go @@ -81,8 +81,9 @@ type ServiceUserBackfiller interface { // AdminConfig is platform administration configuration type AdminConfig struct { // Bootstrap seeds a superuser service account from config (client_id/secret) so - // automation (e.g. GitOps) has a superuser identity without a chicken-and-egg. - // All other platform-user management is handled out-of-band via GitOps reconcile. + // automation (e.g. GitOps) always has a superuser to log in as. This means you + // don't need an existing superuser to create the first one. Everything else about + // platform users is managed separately, through the GitOps reconcile flow. Bootstrap SuperUserBootstrapConfig `yaml:"bootstrap" mapstructure:"bootstrap"` } diff --git a/internal/reconcile/platformuser.go b/internal/reconcile/platformuser.go index db7e4e614..f1b6cebaa 100644 --- a/internal/reconcile/platformuser.go +++ b/internal/reconcile/platformuser.go @@ -42,9 +42,9 @@ const ( opRemove opAction = "remove" ) -// Op is a single planned change against one (principal, relation): add ensures the -// relation, remove strips just that relation (relation-selective). Ref is the desired -// entry's ref for add (email/id) and the current principal's id for remove. +// Op is a single planned change to one (principal, relation). "add" grants the +// relation; "remove" takes away just that one relation. Ref holds the desired +// entry's ref for an add (email or id), and the current principal's id for a remove. type Op struct { Action opAction Type string @@ -82,11 +82,11 @@ func validateSpec(s PlatformUserSpec) error { if strings.TrimSpace(s.Ref) == "" { return fmt.Errorf("empty ref") } - // The bootstrap SA is server-managed (seeded at boot, removal-guarded); it must - // not be reconciled, so reject it on the desired side too — not just skipped on - // the current side. + // The server manages the bootstrap SA (it seeds it at boot and blocks removal), + // so it must not be reconciled. Reject it on the desired side too, not just skip + // it on the current side. if s.Type == principalTypeServiceUser && strings.TrimSpace(s.Ref) == schema.BootstrapServiceUserID { - return fmt.Errorf("ref %q is the bootstrap service account, which is server-managed and cannot be reconciled", s.Ref) + return fmt.Errorf("ref %q is the bootstrap service account, which the server manages and cannot be reconciled", s.Ref) } return nil } @@ -103,13 +103,14 @@ func specMatchesPrincipal(s PlatformUserSpec, p platformPrincipal) bool { return p.Type == principalTypeUser && s.Ref != "" && strings.EqualFold(s.Ref, p.Email) } -// platformRelationOrder gives operations a stable, deterministic order. +// platformRelationOrder gives operations a stable, fixed order. var platformRelationOrder = []string{schema.AdminRelationName, schema.MemberRelationName} -// diffPlatformUsers converges current platform principals to the desired spec at -// the (principal, relation) granularity: each current relation no longer desired is -// removed (relation-selectively), each desired relation not present is added. Desired -// entries matching no current principal are new and are added. Output is deterministic. +// diffPlatformUsers works out the changes needed to make the current platform +// principals match the desired spec, one (principal, relation) at a time. A current +// relation that is no longer wanted is removed. A wanted relation that is missing is +// added. A desired entry that matches no current principal is new, so it is added. +// The output order is fixed. func diffPlatformUsers(desired []PlatformUserSpec, current []platformPrincipal) ([]Op, error) { for _, s := range desired { if err := validateSpec(s); err != nil { @@ -117,10 +118,10 @@ func diffPlatformUsers(desired []PlatformUserSpec, current []platformPrincipal) } } - // Collect adds and removes separately so adds can be emitted first (see the - // return). Apply is sequential, so an add-before-remove order means a role - // transition (e.g. admin→member) never leaves a principal with no platform - // access if a later op fails. + // Collect adds and removes separately so adds come first (see the return). + // Apply runs in order, so doing adds before removes means a relation change + // (e.g. admin -> member) never leaves someone with no platform access if a + // later op fails. var adds, removes []Op matched := make([]bool, len(desired)) diff --git a/internal/reconcile/platformuser_reconciler.go b/internal/reconcile/platformuser_reconciler.go index 298fe73aa..8d592ec02 100644 --- a/internal/reconcile/platformuser_reconciler.go +++ b/internal/reconcile/platformuser_reconciler.go @@ -20,7 +20,7 @@ type PlatformUserAPI interface { RemovePlatformUser(context.Context, *connect.Request[frontierv1beta1.RemovePlatformUserRequest]) (*connect.Response[frontierv1beta1.RemovePlatformUserResponse], error) } -// PlatformUserReconciler converges platform admins/members to the desired spec. +// PlatformUserReconciler makes platform admins and members match the desired spec. type PlatformUserReconciler struct { client PlatformUserAPI header string // "key:value" auth header applied to each request (may be empty) @@ -79,9 +79,9 @@ func (r *PlatformUserReconciler) fetchCurrent(ctx context.Context) ([]platformPr }) } for _, su := range resp.Msg.GetServiceusers() { - // The config-bootstrapped break-glass SA has a fixed, well-known id; it is - // server-managed (seeded at boot, removal-guarded), so never reconcile — and - // so never try to remove — it. + // The bootstrap SA (created from config at boot) has a fixed id. The server + // manages it and blocks removal, so never reconcile it — that way we never + // try to remove it. if su.GetId() == schema.BootstrapServiceUserID { continue } @@ -104,7 +104,7 @@ func (r *PlatformUserReconciler) apply(ctx context.Context, op Op) error { }, r.header)) return err case opRemove: - // relation-selective removal (proton #489): strip only op.Relation. + // remove only op.Relation, not both (proton #489). _, err := r.client.RemovePlatformUser(ctx, authReq(&frontierv1beta1.RemovePlatformUserRequest{ UserId: userID, ServiceuserId: serviceUserID, Relation: op.Relation, }, r.header)) @@ -114,9 +114,9 @@ func (r *PlatformUserReconciler) apply(ctx context.Context, op Op) error { } } -// relationsFromMetadata reads the platform relations ListPlatformUsers stamps into -// each principal's metadata. It prefers the full "relations" list (so a principal -// holding both admin and member is reconciled exactly) and falls back to the +// relationsFromMetadata reads the platform relations that ListPlatformUsers records +// in each principal's metadata. It uses the full "relations" list when present (so +// someone holding both admin and member is handled correctly), and falls back to the // single "relation" field for backward compatibility. func relationsFromMetadata(md *structpb.Struct) map[string]struct{} { rels := map[string]struct{}{} diff --git a/internal/reconcile/platformuser_reconciler_test.go b/internal/reconcile/platformuser_reconciler_test.go index 951fcf800..8bf4f8634 100644 --- a/internal/reconcile/platformuser_reconciler_test.go +++ b/internal/reconcile/platformuser_reconciler_test.go @@ -105,7 +105,7 @@ func TestPlatformUserReconciler_Reconcile(t *testing.T) { assert.Empty(t, api.removed) }) - t.Run("no changes when already converged", func(t *testing.T) { + t.Run("no changes when already matching", func(t *testing.T) { api := &fakePlatformUserAPI{users: []*frontierv1beta1.User{ platformUserPB(t, "new-id", "new@x.com", schema.AdminRelationName), }} @@ -117,9 +117,9 @@ func TestPlatformUserReconciler_Reconcile(t *testing.T) { assert.Empty(t, api.removed) }) - t.Run("never removes the bootstrap service account (well-known id)", func(t *testing.T) { - // empty desired state => fully authoritative removal, but the bootstrap SA - // is matched by its well-known id and must be excluded and left untouched. + t.Run("never removes the bootstrap service account (fixed id)", func(t *testing.T) { + // an empty desired state would remove everyone, but the bootstrap SA is + // matched by its fixed id and must be left out and untouched. api := &fakePlatformUserAPI{sus: []*frontierv1beta1.ServiceUser{ serviceUserPBRelations(t, schema.BootstrapServiceUserID, schema.AdminRelationName), }} diff --git a/internal/reconcile/platformuser_test.go b/internal/reconcile/platformuser_test.go index bbac1421e..cd850b809 100644 --- a/internal/reconcile/platformuser_test.go +++ b/internal/reconcile/platformuser_test.go @@ -43,7 +43,7 @@ func TestDiffPlatformUsers(t *testing.T) { assert.Equal(t, []Op{{Action: opAdd, Type: "serviceuser", Ref: "su-1", Relation: member}}, ops) }) - t.Run("already-correct principal is a no-op (matched by email)", func(t *testing.T) { + t.Run("already-correct principal makes no change (matched by email)", func(t *testing.T) { ops, err := diffPlatformUsers( []PlatformUserSpec{{Type: "user", Ref: "alice@x.com", Relation: admin}}, []platformPrincipal{principal("user", "alice-id", "alice@x.com", admin)}, @@ -81,7 +81,7 @@ func TestDiffPlatformUsers(t *testing.T) { {Type: "user", Ref: "new@x.com", Relation: member}, // new -> add }, []platformPrincipal{ - principal("user", "keep-id", "keep@x.com", admin), // matches keep -> no-op + principal("user", "keep-id", "keep@x.com", admin), // matches keep -> no change principal("user", "drop-id", "drop@x.com", admin), // not desired -> remove }, ) @@ -129,7 +129,7 @@ func TestDiffPlatformUsers(t *testing.T) { assert.Error(t, err) }) - t.Run("rejects the server-managed bootstrap service account", func(t *testing.T) { + t.Run("rejects the bootstrap service account (the server manages it)", func(t *testing.T) { _, err := diffPlatformUsers( []PlatformUserSpec{{Type: "serviceuser", Ref: schema.BootstrapServiceUserID, Relation: admin}}, nil) assert.Error(t, err) @@ -164,8 +164,8 @@ func TestSpecRefMatching(t *testing.T) { }) // end-to-end through the diff: a user referenced by id and a service user by id - // both converge to no-ops when already in the desired relation. - t.Run("diff converges: user by id + serviceuser by id already correct", func(t *testing.T) { + // both make no change when already in the desired relation. + t.Run("diff makes no change: user by id + serviceuser by id already correct", func(t *testing.T) { ops, err := diffPlatformUsers( []PlatformUserSpec{ {Type: "user", Ref: "user-uuid-1", Relation: admin}, diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go index 2a6f42262..0ebae019a 100644 --- a/internal/reconcile/reconcile.go +++ b/internal/reconcile/reconcile.go @@ -1,8 +1,8 @@ -// Package reconcile converges Frontier platform resources to a declarative -// desired-state file via the admin API. It is a small framework: each resource -// kind implements Reconciler and registers under its kind, so new kinds -// (roles, plans, products, traits, ...) plug in without changing the command -// or file format. PlatformUser is the first kind. +// Package reconcile makes Frontier platform resources match a desired-state +// file, through the admin API. It is a small framework: each resource kind +// implements Reconciler and registers under its kind, so new kinds (roles, +// plans, products, traits, ...) plug in without changing the command or the +// file format. PlatformUser is the first kind. package reconcile import ( @@ -15,7 +15,7 @@ import ( "gopkg.in/yaml.v3" ) -// Reconciler converges a single resource kind from its desired-state spec. +// Reconciler makes a single resource kind match its desired-state spec. type Reconciler interface { Kind() string Reconcile(ctx context.Context, spec []byte, dryRun bool) (Report, error) @@ -35,9 +35,9 @@ type document struct { Spec yaml.Node `yaml:"spec"` } -// Run parses a (possibly multi-document) desired-state file and dispatches each -// document to the registered reconciler for its kind. Documents apply in file -// order; the first error aborts and returns the reports gathered so far. +// Run parses a (possibly multi-document) desired-state file and sends each +// document to the reconciler registered for its kind. Documents apply in file +// order. The first error stops the run and returns the reports gathered so far. func Run(ctx context.Context, registry map[string]Reconciler, data []byte, dryRun bool) ([]Report, error) { dec := yaml.NewDecoder(bytes.NewReader(data)) var reports []Report @@ -61,9 +61,9 @@ func Run(ctx context.Context, registry map[string]Reconciler, data []byte, dryRu if err != nil { return reports, fmt.Errorf("marshal spec for kind %q: %w", doc.Kind, err) } - // An absent or null spec marshals to "null"/"" — almost always a typo (e.g. - // `spce:`). Never treat that as an authoritative empty state that would strip - // every principal; an intentional empty is `spec: []`. + // A missing or null spec marshals to "null"/"" — almost always a typo (e.g. + // `spce:`). Don't treat that as an empty list that would remove every user; + // an empty list you meant to write is `spec: []`. if s := strings.TrimSpace(string(specBytes)); s == "" || s == "null" { return reports, fmt.Errorf("document kind %q is missing its spec", doc.Kind) } diff --git a/internal/reconcile/reconcile_test.go b/internal/reconcile/reconcile_test.go index 50127344f..1d6802ba8 100644 --- a/internal/reconcile/reconcile_test.go +++ b/internal/reconcile/reconcile_test.go @@ -16,7 +16,7 @@ func (f *fakeReconciler) Reconcile(_ context.Context, _ []byte, _ bool) (Report, } func TestRun_SpecHandling(t *testing.T) { - t.Run("rejects a document missing its spec (not an authoritative empty state)", func(t *testing.T) { + t.Run("rejects a document missing its spec (not an empty list)", func(t *testing.T) { rec := &fakeReconciler{} reg := map[string]Reconciler{KindPlatformUser: rec}