From a226f47c29d65f5f32814ce8d7ac630c80dbe4dd Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Thu, 16 Jul 2026 13:58:28 +0530 Subject: [PATCH 1/3] feat(reconcile): add Permission and Role kinds, bootstrap only creates missing roles --- cmd/export.go | 3 +- cmd/reconcile.go | 37 +- cmd/serve.go | 4 + config/sample.config.yaml | 2 + docs/content/docs/reconcile.mdx | 80 ++++- docs/content/docs/reference/cli.mdx | 10 +- internal/api/v1beta1connect/role.go | 3 + internal/api/v1beta1connect/role_test.go | 11 + internal/bootstrap/service.go | 58 +--- internal/bootstrap/service_test.go | 58 +--- internal/reconcile/permission.go | 127 +++++++ internal/reconcile/permission_reconciler.go | 125 +++++++ .../reconcile/permission_reconciler_test.go | 98 ++++++ internal/reconcile/permission_test.go | 88 +++++ internal/reconcile/reconcile.go | 16 + internal/reconcile/role.go | 315 ++++++++++++++++++ internal/reconcile/role_reconciler.go | 199 +++++++++++ internal/reconcile/role_reconciler_test.go | 285 ++++++++++++++++ internal/reconcile/role_test.go | 246 ++++++++++++++ 19 files changed, 1645 insertions(+), 120 deletions(-) create mode 100644 internal/reconcile/permission.go create mode 100644 internal/reconcile/permission_reconciler.go create mode 100644 internal/reconcile/permission_reconciler_test.go create mode 100644 internal/reconcile/permission_test.go create mode 100644 internal/reconcile/role.go create mode 100644 internal/reconcile/role_reconciler.go create mode 100644 internal/reconcile/role_reconciler_test.go create mode 100644 internal/reconcile/role_test.go diff --git a/cmd/export.go b/cmd/export.go index fe27a4b81..1ad743ec4 100644 --- a/cmd/export.go +++ b/cmd/export.go @@ -31,11 +31,10 @@ func ExportCommand(cliConfig *Config) *cli.Command { }, Args: cli.ExactArgs(1), RunE: func(cmd *cli.Command, args []string) error { - adminClient, err := createAdminClient(cliConfig.Host) + registry, err := buildReconcileRegistry(cliConfig.Host, header) if err != nil { return err } - registry := reconcileRegistry(adminClient, header) kind, err := resolveKind(args[0], registry) if err != nil { return err diff --git a/cmd/reconcile.go b/cmd/reconcile.go index 9fbd63200..4b2ecff81 100644 --- a/cmd/reconcile.go +++ b/cmd/reconcile.go @@ -22,9 +22,11 @@ func ReconcileCommand(cliConfig *Config) *cli.Command { Long: heredoc.Doc(` Make platform resources match a desired-state YAML file, through the admin API. - 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. + Kinds: PlatformUser (platform admins and members), Permission (custom + permissions), and Role (platform-level roles). Deleting a permission or a + role needs an explicit 'delete: true' on its entry; nothing is deleted by + omission. Log in as a superuser (for example the bootstrap service account) + with --header. Use "frontier export " to print the current state in this file format. `), @@ -41,11 +43,11 @@ func ReconcileCommand(cliConfig *Config) *cli.Command { if err != nil { return fmt.Errorf("read desired-state file: %w", err) } - adminClient, err := createAdminClient(cliConfig.Host) + registry, err := buildReconcileRegistry(cliConfig.Host, header) if err != nil { return err } - reports, runErr := reconcile.Run(cmd.Context(), reconcileRegistry(adminClient, header), data, dryRun) + reports, runErr := reconcile.Run(cmd.Context(), registry, data, dryRun) for _, rep := range reports { printReconcileReport(cmd, rep) } @@ -60,11 +62,30 @@ func ReconcileCommand(cliConfig *Config) *cli.Command { return cmd } -// reconcileRegistry holds every reconcilable kind. New kinds register here. -func reconcileRegistry(adminClient frontierv1beta1connect.AdminServiceClient, header string) map[string]reconcile.Reconciler { +// reconcileAPI joins the two generated clients: reads live on FrontierService, +// writes on AdminService. The embedded method sets combine to satisfy the +// per-kind API interfaces. +type reconcileAPI struct { + frontierv1beta1connect.AdminServiceClient + frontierv1beta1connect.FrontierServiceClient +} + +// buildReconcileRegistry holds every reconcilable kind. New kinds register here. +func buildReconcileRegistry(host, header string) (map[string]reconcile.Reconciler, error) { + adminClient, err := createAdminClient(host) + if err != nil { + return nil, err + } + frontierClient, err := createClient(host) + if err != nil { + return nil, err + } + api := reconcileAPI{AdminServiceClient: adminClient, FrontierServiceClient: frontierClient} return map[string]reconcile.Reconciler{ reconcile.KindPlatformUser: reconcile.NewPlatformUserReconciler(adminClient, header), - } + reconcile.KindPermission: reconcile.NewPermissionReconciler(api, header), + reconcile.KindRole: reconcile.NewRoleReconciler(api, header), + }, nil } // printReconcileReport writes the report to stdout. cobra's cmd.Printf falls diff --git a/cmd/serve.go b/cmd/serve.go index 00ad5f077..5c3f77e20 100644 --- a/cmd/serve.go +++ b/cmd/serve.go @@ -136,6 +136,10 @@ func StartServer(logger *slog.Logger, cfg *config.Frontier) error { }() // load resource config + if cfg.App.ResourcesConfigPath != "" { + logger.Warn("app.resources_config_path is deprecated and will be removed after a deprecation window; " + + "manage permissions and roles with 'frontier reconcile' instead") + } resourceBlobFS, err := blob.NewStore(ctx, cfg.App.ResourcesConfigPath, cfg.App.ResourcesConfigPathSecret) if err != nil { return err diff --git a/config/sample.config.yaml b/config/sample.config.yaml index aa080121b..78c772067 100644 --- a/config/sample.config.yaml +++ b/config/sample.config.yaml @@ -63,6 +63,8 @@ app: host: "127.0.0.1" # WARNING: identity_proxy_header bypass all authorization checks and shouldn't be used in production identity_proxy_header: X-Frontier-Email + # DEPRECATED: will be removed after a deprecation window. Manage permissions + # and roles with 'frontier reconcile' instead (see docs/rfcs/0001-declarative-reconcile.md). # full path prefixed with scheme where resources config yaml files are kept # e.g.: # local storage file "file:///tmp/resources_config" diff --git a/docs/content/docs/reconcile.mdx b/docs/content/docs/reconcile.mdx index 038289095..4dccc2cd4 100644 --- a/docs/content/docs/reconcile.mdx +++ b/docs/content/docs/reconcile.mdx @@ -47,6 +47,9 @@ Rules that apply to every document: - The whole file is parsed and checked before anything applies. A malformed later document stops the run before any change is made. - Documents apply in the order they appear in the file. +- Some kinds depend on others. A role can grant a permission, so a `Permission` document + must come before a `Role` document in the same file. The wrong order is rejected before + anything applies. ## The PlatformUser kind @@ -73,6 +76,75 @@ Two safety properties: Adding a user by an email that does not exist creates that user. +## The Permission kind + +A permission is an identity: a `service/resource` namespace plus a verb. There is nothing +else to manage on it, so it is either created or deleted. + +```yaml +apiVersion: v1 +kind: Permission +spec: + - namespace: compute/order + name: get + - namespace: compute/order + name: legacy + delete: true +``` + +- The namespace has two parts, `service/resource`. The name is alphanumeric. +- Frontier's own permissions (the `app` namespaces) belong to the base schema. The + reconciler ignores them and rejects a file entry that names one. +- A custom permission on the server that the file does not list fails the plan. Nothing + is deleted just because it is missing; deleting needs `delete: true` on the entry. +- Creating a permission also updates the authorization schema, so a new permission is + ready to use in roles right away. + +## The Role kind + +`Role` manages platform-level roles. The role name is the identity and never changes. The +managed fields are the title, the description, the permissions, and the scopes (the +resource types a role can attach to). + +```yaml +apiVersion: v1 +kind: Role +spec: + - name: compute_order_manager # custom role: must list its permissions + title: Order Manager + description: Manages compute orders + permissions: + - compute_order_get + - compute_order_update + - name: app_project_viewer # predefined role: override only the fields you list + permissions: + - app_project_get + - resource_aoi_get + - name: old_role + delete: true +``` + +There are two kinds of roles, and they behave differently. + +**Custom roles** are yours to manage in full. A custom role must list at least one +permission. Every custom role on the server must appear in the file, kept or marked +`delete: true`; one that is missing fails the plan. A role that still has policy bindings +cannot be deleted, and the apply fails with a clear error. + +**Predefined roles** are the ones Frontier ships, like `app_organization_owner` or +`app_project_viewer`. They converge to their shipped definitions. A file entry overrides +only the fields it lists; a field you leave out goes back to the default. A predefined +role you do not list at all resets fully. The plan marks these resets, so a hand-made +change to a predefined role shows up as an update back to the default. You cannot delete a +predefined role; the server recreates it at boot. + +Because a predefined role resets to its default, removing its entry from the file is a +real change. If an entry adds permissions to `app_project_viewer`, deleting that entry +takes those permissions away on the next apply. + +Permission references accept any form the server knows: the slug (`compute_order_get`), +`service/resource:verb`, or `service.resource.verb`. + ## Running it Log in as a superuser. The bootstrap service user exists for exactly this; its client id @@ -115,8 +187,8 @@ The kind argument is case-insensitive and accepts a plural, so `platformuser` an ## More kinds -The design and the rules every kind follows live in -[RFC 0001](https://github.com/raystack/frontier/blob/main/docs/rfcs/0001-declarative-reconcile.md). -Custom permissions and platform-level roles are proposed there as the next kinds; this -page grows with them. The flag reference for both commands is in the +This page covers `PlatformUser`, `Permission`, and `Role`. The design and the rules every +kind follows live in +[RFC 0001](https://github.com/raystack/frontier/blob/main/docs/rfcs/0001-declarative-reconcile.md), +which also lists the kinds proposed next. The flag reference for both commands is in the [CLI reference](./reference/cli.md). diff --git a/docs/content/docs/reference/cli.mdx b/docs/content/docs/reference/cli.mdx index 533c12f20..f8e458817 100644 --- a/docs/content/docs/reference/cli.mdx +++ b/docs/content/docs/reference/cli.mdx @@ -33,7 +33,7 @@ List of supported environment variables Export the current state of a kind as a desired-state YAML file, printed to stdout. The output is the format `frontier reconcile` reads: reconciling it -changes nothing. Supported kind: `PlatformUser`. See the +changes nothing. Supported kinds: `PlatformUser`, `Permission`, `Role`. See the [Reconcile guide](../reconcile.md) for the file format and the flow. ``` @@ -246,9 +246,11 @@ View a project ## `frontier reconcile [flags]` Make platform resources match a desired-state YAML file, through the admin -API. The file decides who has access: anyone listed is added, anyone not -listed is removed. Supported kind: `PlatformUser`. Use `frontier export` to -print the current state in this file format, and see the +API. Supported kinds: `PlatformUser` (anyone listed is added, anyone not +listed is removed), `Permission` (custom permissions), and `Role` +(platform-level roles). Deleting a permission or a role needs an explicit +`delete: true` on its entry; nothing is deleted by omission. Use +`frontier export` to print the current state in this file format, and see the [Reconcile guide](../reconcile.md) for the full flow. ``` diff --git a/internal/api/v1beta1connect/role.go b/internal/api/v1beta1connect/role.go index 748fb9f6f..545c3ae6c 100644 --- a/internal/api/v1beta1connect/role.go +++ b/internal/api/v1beta1connect/role.go @@ -27,6 +27,9 @@ func (h *ConnectHandler) CreateRole(ctx context.Context, request *connect.Reques if request.Msg.GetBody() == nil { return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) } + if len(request.Msg.GetBody().GetPermissions()) == 0 { + return nil, connect.NewError(connect.CodeInvalidArgument, ErrBadRequest) + } var metaDataMap metadata.Metadata if request.Msg.GetBody().GetMetadata() != nil { diff --git a/internal/api/v1beta1connect/role_test.go b/internal/api/v1beta1connect/role_test.go index 28e84cbca..64a3c8aff 100644 --- a/internal/api/v1beta1connect/role_test.go +++ b/internal/api/v1beta1connect/role_test.go @@ -84,6 +84,17 @@ func TestHandler_CreateRole(t *testing.T) { want: nil, wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), }, + { + name: "should return bad request error if permissions are empty", + request: connect.NewRequest(&frontierv1beta1.CreateRoleRequest{ + Body: &frontierv1beta1.RoleRequestBody{ + Name: testRoleMap[instanceLevelRoleID].Name, + Permissions: nil, + }, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, ErrBadRequest), + }, { name: "should create role on success", setup: func(rs *mocks.RoleService, ms *mocks.MetaSchemaService) { diff --git a/internal/bootstrap/service.go b/internal/bootstrap/service.go index 6c4866bf8..a4d9dd26b 100644 --- a/internal/bootstrap/service.go +++ b/internal/bootstrap/service.go @@ -35,7 +35,6 @@ type RoleService interface { Get(ctx context.Context, id string) (role.Role, error) List(ctx context.Context, f role.Filter) ([]role.Role, error) Upsert(ctx context.Context, toCreate role.Role) (role.Role, error) - Update(ctx context.Context, toUpdate role.Role) (role.Role, error) } type RelationService interface { @@ -288,20 +287,21 @@ func (s Service) MigrateRoles(ctx context.Context) error { return nil } -// migrateRole makes the stored role match its definition: create when missing, -// reconcile when present. Roles are keyed by name, so renaming a definition -// produces a new role rather than renaming the existing one. +// migrateRole creates the role when it is missing and leaves it alone when it +// exists. Operators manage existing roles (title, permissions) out of band via +// the reconcile flow; updating them here would revert those changes on every +// boot. func (s Service) migrateRole(ctx context.Context, orgID string, defRole schema.RoleDefinition) error { - existing, err := s.roleService.Get(ctx, defRole.Name) + _, err := s.roleService.Get(ctx, defRole.Name) if errors.Is(err, role.ErrNotExist) { return s.createRole(ctx, orgID, defRole) } if err != nil { // A transient Get failure must not fall through to create: that would - // re-Upsert an existing role and skip the prune, the very drift this fixes. + // re-Upsert an existing role, reverting out-of-band changes. return fmt.Errorf("get role %s: %w: %s", defRole.Name, schema.ErrMigration, err.Error()) } - return s.reconcileRole(ctx, existing, defRole) + return nil } func (s Service) createRole(ctx context.Context, orgID string, defRole schema.RoleDefinition) error { @@ -322,50 +322,6 @@ func (s Service) createRole(ctx context.Context, orgID string, defRole schema.Ro return nil } -// reconcileRole writes the definition onto an existing role only when its -// permission set differs. The write goes through role.Update (not a raw row -// update) because that is what prunes the SpiceDB permission tuples a narrowed -// definition no longer grants; the equality short-circuit keeps every boot from -// rewriting unchanged roles and emitting a spurious RoleUpdated audit record. -func (s Service) reconcileRole(ctx context.Context, existing role.Role, defRole schema.RoleDefinition) error { - if permissionsEqual(existing.Permissions, defRole.Permissions) { - return nil - } - existing.Title = defRole.Title - existing.Permissions = defRole.Permissions - existing.Scopes = defRole.Scopes - if existing.Metadata == nil { - existing.Metadata = map[string]any{} - } - existing.Metadata["description"] = defRole.Description - if _, err := s.roleService.Update(ctx, existing); err != nil { - return fmt.Errorf("can't reconcile role: %w: %s", schema.ErrMigration, err.Error()) - } - return nil -} - -// permissionsEqual reports whether two permission slug sets are equal, ignoring -// order and duplicates. -func permissionsEqual(a, b []string) bool { - setA := make(map[string]struct{}, len(a)) - for _, p := range a { - setA[p] = struct{}{} - } - setB := make(map[string]struct{}, len(b)) - for _, p := range b { - setB[p] = struct{}{} - } - if len(setA) != len(setB) { - return false - } - for p := range setB { - if _, ok := setA[p]; !ok { - return false - } - } - return true -} - // MigrateServiceUserOrgPolicies backfills the org policy for service users that // have only a SpiceDB member relation (legacy creation flow). Idempotent: on a // clean cluster the candidate query returns zero rows and this is a no-op. diff --git a/internal/bootstrap/service_test.go b/internal/bootstrap/service_test.go index 68c8f2784..28c1f78c5 100644 --- a/internal/bootstrap/service_test.go +++ b/internal/bootstrap/service_test.go @@ -261,13 +261,15 @@ func Test_migrateRole(t *testing.T) { roleSvc.AssertNotCalled(t, "Update") }) - t.Run("skips reconcile when the permission set is unchanged", func(t *testing.T) { + t.Run("leaves an existing role alone even when it differs from the definition", func(t *testing.T) { roleSvc := new(mockRoleService) roleSvc.On("Get", mock.Anything, def.Name).Return(role.Role{ - ID: "role-1", - Name: def.Name, - // same set, different order -> still equal - Permissions: []string{"app_organization_update", "app_organization_get"}, + ID: "role-1", + Name: def.Name, + Title: "Renamed By Operator", + // differs from the definition; operators own existing roles, so + // boot must not write it back + Permissions: []string{"app_organization_get"}, }, nil) svc := Service{roleService: roleSvc} @@ -276,24 +278,6 @@ func Test_migrateRole(t *testing.T) { roleSvc.AssertNotCalled(t, "Update") }) - t.Run("reconciles a drifted (over-granting) role to the definition", func(t *testing.T) { - roleSvc := new(mockRoleService) - roleSvc.On("Get", mock.Anything, def.Name).Return(role.Role{ - ID: "role-1", - Name: def.Name, - // has an extra permission no longer in the definition - Permissions: []string{"app_organization_get", "app_organization_update", "app_organization_delete"}, - }, nil) - roleSvc.On("Update", mock.Anything, mock.MatchedBy(func(r role.Role) bool { - return r.ID == "role-1" && len(r.Permissions) == 2 && - !contains(r.Permissions, "app_organization_delete") - })).Return(role.Role{ID: "role-1"}, nil) - - svc := Service{roleService: roleSvc} - assert.NoError(t, svc.migrateRole(context.Background(), "org-1", def)) - roleSvc.AssertNotCalled(t, "Upsert") - }) - t.Run("propagates a transient Get error instead of creating", func(t *testing.T) { roleSvc := new(mockRoleService) // a non-ErrNotExist failure must not fall through to Upsert/Update @@ -306,31 +290,3 @@ func Test_migrateRole(t *testing.T) { roleSvc.AssertNotCalled(t, "Update") }) } - -func contains(s []string, v string) bool { - for _, e := range s { - if e == v { - return true - } - } - return false -} - -func Test_permissionsEqual(t *testing.T) { - cases := []struct { - name string - a, b []string - want bool - }{ - {"equal ignoring order", []string{"x", "y"}, []string{"y", "x"}, true}, - {"equal ignoring duplicates", []string{"x", "x"}, []string{"x"}, true}, - {"different members", []string{"x", "y"}, []string{"x", "z"}, false}, - {"superset", []string{"x"}, []string{"x", "y"}, false}, - {"both empty", nil, []string{}, true}, - } - for _, c := range cases { - t.Run(c.name, func(t *testing.T) { - assert.Equal(t, c.want, permissionsEqual(c.a, c.b)) - }) - } -} diff --git a/internal/reconcile/permission.go b/internal/reconcile/permission.go new file mode 100644 index 000000000..33616efa9 --- /dev/null +++ b/internal/reconcile/permission.go @@ -0,0 +1,127 @@ +package reconcile + +import ( + "fmt" + "sort" + "strings" + + "github.com/raystack/frontier/internal/bootstrap/schema" +) + +// KindPermission is the desired-state document kind for custom permissions. +const KindPermission = "Permission" + +// PermissionSpec is one desired permission. A permission is identity only +// (namespace + name): it is added or deleted, never updated. Deleting needs +// the explicit flag; a permission that just disappears from the file fails +// the plan instead. +type PermissionSpec struct { + Namespace string `yaml:"namespace"` + Name string `yaml:"name"` + Delete bool `yaml:"delete,omitempty"` +} + +func (s PermissionSpec) String() string { + return s.Namespace + ":" + s.Name +} + +func (s PermissionSpec) slug() string { + return schema.FQPermissionNameFromNamespace(s.Namespace, s.Name) +} + +// isBaseNamespace reports whether a namespace belongs to the base schema, +// which the server manages itself. +func isBaseNamespace(ns string) bool { + return ns == "app" || strings.HasPrefix(ns, "app/") +} + +func validatePermissionSpec(s PermissionSpec) error { + if strings.TrimSpace(s.Namespace) == "" || strings.TrimSpace(s.Name) == "" { + return fmt.Errorf("namespace and name are required") + } + if !schema.IsValidPermissionName(s.Name) { + return fmt.Errorf("invalid name %q (alphanumeric only)", s.Name) + } + if isBaseNamespace(s.Namespace) { + return fmt.Errorf("namespace %q is part of the base schema, which the server manages", s.Namespace) + } + if parts := strings.Split(s.Namespace, "/"); len(parts) != 2 || parts[0] == "" || parts[1] == "" { + return fmt.Errorf("namespace %q must be in service/resource form", s.Namespace) + } + return nil +} + +// currentPermission is one custom permission as returned by ListPermissions. +type currentPermission struct { + ID string + Namespace string + Name string +} + +type permissionOp struct { + action opAction + spec PermissionSpec + id string // server row id, set for deletes +} + +func (o permissionOp) String() string { + if o.action == opRemove { + return fmt.Sprintf("delete permission %s", o.spec) + } + return fmt.Sprintf("add permission %s", o.spec) +} + +// diffPermissions returns the ops that make the current custom permissions +// match the desired spec. Every custom permission on the server must appear in +// the spec — kept, or marked delete — so nothing is ever removed by omission. +func diffPermissions(desired []PermissionSpec, current []currentPermission) ([]permissionOp, error) { + bySlug := make(map[string]currentPermission, len(current)) + for _, c := range current { + bySlug[schema.FQPermissionNameFromNamespace(c.Namespace, c.Name)] = c + } + + // The slug is the identity the server enforces (unique in the database), so + // the diff is keyed by it. Distinct namespace+name pairs can flatten to the + // same slug when a namespace part contains underscores; that is a conflict. + seen := map[string]PermissionSpec{} + accounted := map[string]struct{}{} + var adds, removes []permissionOp + for _, s := range desired { + if err := validatePermissionSpec(s); err != nil { + return nil, fmt.Errorf("invalid permission spec %s: %w", s, err) + } + slug := s.slug() + if prev, dup := seen[slug]; dup { + if prev.Namespace != s.Namespace || prev.Name != s.Name { + return nil, fmt.Errorf("permissions %s and %s collide on the same slug %q", prev, s, slug) + } + if prev.Delete != s.Delete { + return nil, fmt.Errorf("permission %s is listed both with and without delete", s) + } + continue + } + seen[slug] = s + accounted[slug] = struct{}{} + + cur, exists := bySlug[slug] + switch { + case s.Delete && exists: + removes = append(removes, permissionOp{action: opRemove, spec: s, id: cur.ID}) + case !s.Delete && !exists: + adds = append(adds, permissionOp{action: opAdd, spec: s}) + } + } + + var unaccounted []string + for slug, c := range bySlug { + if _, ok := accounted[slug]; !ok { + unaccounted = append(unaccounted, c.Namespace+":"+c.Name) + } + } + if len(unaccounted) > 0 { + sort.Strings(unaccounted) + return nil, fmt.Errorf("permissions exist on the server but are not in the file: %s; keep them or mark them 'delete: true'", strings.Join(unaccounted, ", ")) + } + + return append(adds, removes...), nil +} diff --git a/internal/reconcile/permission_reconciler.go b/internal/reconcile/permission_reconciler.go new file mode 100644 index 000000000..c35c72508 --- /dev/null +++ b/internal/reconcile/permission_reconciler.go @@ -0,0 +1,125 @@ +package reconcile + +import ( + "context" + "fmt" + "sort" + + "connectrpc.com/connect" + "github.com/raystack/frontier/internal/bootstrap/schema" + frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" + "gopkg.in/yaml.v3" +) + +// PermissionAPI is the API subset the permission reconciler needs. Reads live +// on FrontierService, writes on AdminService; the caller provides one value +// that serves both. +type PermissionAPI interface { + ListPermissions(context.Context, *connect.Request[frontierv1beta1.ListPermissionsRequest]) (*connect.Response[frontierv1beta1.ListPermissionsResponse], error) + CreatePermission(context.Context, *connect.Request[frontierv1beta1.CreatePermissionRequest]) (*connect.Response[frontierv1beta1.CreatePermissionResponse], error) + DeletePermission(context.Context, *connect.Request[frontierv1beta1.DeletePermissionRequest]) (*connect.Response[frontierv1beta1.DeletePermissionResponse], error) +} + +// PermissionReconciler makes custom permissions match the desired spec. +// Base-schema permissions (app namespaces) are server-managed and ignored. +type PermissionReconciler struct { + client PermissionAPI + header string +} + +func NewPermissionReconciler(client PermissionAPI, header string) *PermissionReconciler { + return &PermissionReconciler{client: client, header: header} +} + +func (r *PermissionReconciler) Kind() string { return KindPermission } + +func (r *PermissionReconciler) Reconcile(ctx context.Context, spec []byte, dryRun bool) (Report, error) { + var specs []PermissionSpec + if err := yaml.Unmarshal(spec, &specs); err != nil { + return Report{}, fmt.Errorf("parse %s spec: %w", KindPermission, err) + } + + current, err := r.fetchCurrent(ctx) + if err != nil { + return Report{}, err + } + + ops, err := diffPermissions(specs, current) + if err != nil { + return Report{}, err + } + + rep := Report{Kind: KindPermission, 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 +} + +// Export returns the current custom permissions as a desired-state spec, +// sorted so repeated exports produce identical files. +func (r *PermissionReconciler) Export(ctx context.Context) (any, error) { + current, err := r.fetchCurrent(ctx) + if err != nil { + return nil, err + } + specs := make([]PermissionSpec, 0, len(current)) + for _, c := range current { + specs = append(specs, PermissionSpec{Namespace: c.Namespace, Name: c.Name}) + } + sort.Slice(specs, func(i, j int) bool { + a, b := specs[i], specs[j] + if a.Namespace != b.Namespace { + return a.Namespace < b.Namespace + } + return a.Name < b.Name + }) + return specs, nil +} + +func (r *PermissionReconciler) fetchCurrent(ctx context.Context) ([]currentPermission, error) { + resp, err := r.client.ListPermissions(ctx, authReq(&frontierv1beta1.ListPermissionsRequest{}, r.header)) + if err != nil { + return nil, fmt.Errorf("list permissions: %w", err) + } + var current []currentPermission + for _, p := range resp.Msg.GetPermissions() { + if isBaseNamespace(p.GetNamespace()) { + continue + } + current = append(current, currentPermission{ + ID: p.GetId(), + Namespace: p.GetNamespace(), + Name: p.GetName(), + }) + } + return current, nil +} + +func (r *PermissionReconciler) apply(ctx context.Context, op permissionOp) error { + switch op.action { + case opAdd: + _, err := r.client.CreatePermission(ctx, authReq(&frontierv1beta1.CreatePermissionRequest{ + Bodies: []*frontierv1beta1.PermissionRequestBody{{ + Key: schema.PermissionKeyFromNamespaceAndName(op.spec.Namespace, op.spec.Name), + }}, + }, r.header)) + return err + case opRemove: + _, err := r.client.DeletePermission(ctx, authReq(&frontierv1beta1.DeletePermissionRequest{ + Id: op.id, + }, r.header)) + return err + default: + return fmt.Errorf("unknown op action %q", op.action) + } +} diff --git a/internal/reconcile/permission_reconciler_test.go b/internal/reconcile/permission_reconciler_test.go new file mode 100644 index 000000000..121d70dce --- /dev/null +++ b/internal/reconcile/permission_reconciler_test.go @@ -0,0 +1,98 @@ +package reconcile + +import ( + "context" + "testing" + + "connectrpc.com/connect" + frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" + "github.com/stretchr/testify/assert" +) + +type fakePermissionAPI struct { + perms []*frontierv1beta1.Permission + created []*frontierv1beta1.CreatePermissionRequest + deleted []string +} + +func (f *fakePermissionAPI) ListPermissions(_ context.Context, _ *connect.Request[frontierv1beta1.ListPermissionsRequest]) (*connect.Response[frontierv1beta1.ListPermissionsResponse], error) { + return connect.NewResponse(&frontierv1beta1.ListPermissionsResponse{Permissions: f.perms}), nil +} + +func (f *fakePermissionAPI) CreatePermission(_ context.Context, req *connect.Request[frontierv1beta1.CreatePermissionRequest]) (*connect.Response[frontierv1beta1.CreatePermissionResponse], error) { + f.created = append(f.created, req.Msg) + return connect.NewResponse(&frontierv1beta1.CreatePermissionResponse{}), nil +} + +func (f *fakePermissionAPI) DeletePermission(_ context.Context, req *connect.Request[frontierv1beta1.DeletePermissionRequest]) (*connect.Response[frontierv1beta1.DeletePermissionResponse], error) { + f.deleted = append(f.deleted, req.Msg.GetId()) + return connect.NewResponse(&frontierv1beta1.DeletePermissionResponse{}), nil +} + +func permissionPB(id, namespace, name string) *frontierv1beta1.Permission { + return &frontierv1beta1.Permission{Id: id, Namespace: namespace, Name: name} +} + +func TestPermissionReconciler(t *testing.T) { + t.Run("applies adds and deletes, base namespaces invisible", func(t *testing.T) { + api := &fakePermissionAPI{perms: []*frontierv1beta1.Permission{ + permissionPB("b1", "app/organization", "administer"), // base: must be ignored + permissionPB("p1", "compute/order", "legacy"), + }} + spec := []byte("- {namespace: compute/order, name: legacy, delete: true}\n- {namespace: compute/order, name: get}\n") + + rep, err := NewPermissionReconciler(api, "").Reconcile(context.Background(), spec, false) + + assert.NoError(t, err) + assert.Equal(t, 2, rep.Applied) + if assert.Len(t, api.created, 1) { + bodies := api.created[0].GetBodies() + if assert.Len(t, bodies, 1) { + assert.Equal(t, "compute.order.get", bodies[0].GetKey()) + } + } + assert.Equal(t, []string{"p1"}, api.deleted) + }) + + t.Run("dry-run plans without applying", func(t *testing.T) { + api := &fakePermissionAPI{} + spec := []byte("- {namespace: compute/order, name: get}\n") + + rep, err := NewPermissionReconciler(api, "").Reconcile(context.Background(), spec, true) + + assert.NoError(t, err) + assert.Equal(t, []string{"add permission compute/order:get"}, rep.Planned) + assert.Zero(t, rep.Applied) + assert.Empty(t, api.created) + }) + + t.Run("reconciling an exported document plans no changes", func(t *testing.T) { + api := &fakePermissionAPI{perms: []*frontierv1beta1.Permission{ + permissionPB("b1", "app/project", "get"), + permissionPB("p2", "compute/order", "update"), + permissionPB("p1", "compute/disk", "mount"), + }} + registry := map[string]Reconciler{KindPermission: NewPermissionReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindPermission) + assert.NoError(t, err) + assert.NotContains(t, string(out), "app/project") + + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + + t.Run("export of no custom permissions yields an empty list", func(t *testing.T) { + api := &fakePermissionAPI{perms: []*frontierv1beta1.Permission{ + permissionPB("b1", "app/organization", "administer"), + }} + registry := map[string]Reconciler{KindPermission: NewPermissionReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindPermission) + assert.NoError(t, err) + assert.Contains(t, string(out), "spec: []") + }) +} diff --git a/internal/reconcile/permission_test.go b/internal/reconcile/permission_test.go new file mode 100644 index 000000000..ade1237e6 --- /dev/null +++ b/internal/reconcile/permission_test.go @@ -0,0 +1,88 @@ +package reconcile + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestDiffPermissions(t *testing.T) { + current := []currentPermission{ + {ID: "p1", Namespace: "compute/order", Name: "get"}, + {ID: "p2", Namespace: "compute/order", Name: "legacy"}, + } + + t.Run("adds missing and deletes flagged, adds first", func(t *testing.T) { + ops, err := diffPermissions([]PermissionSpec{ + {Namespace: "compute/order", Name: "get"}, + {Namespace: "compute/order", Name: "legacy", Delete: true}, + {Namespace: "compute/disk", Name: "mount"}, + }, current) + + assert.NoError(t, err) + if assert.Len(t, ops, 2) { + assert.Equal(t, "add permission compute/disk:mount", ops[0].String()) + assert.Equal(t, "delete permission compute/order:legacy", ops[1].String()) + assert.Equal(t, "p2", ops[1].id) + } + }) + + t.Run("no changes when converged", func(t *testing.T) { + ops, err := diffPermissions([]PermissionSpec{ + {Namespace: "compute/order", Name: "get"}, + {Namespace: "compute/order", Name: "legacy"}, + }, current) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + t.Run("delete of an absent permission is a no-op", func(t *testing.T) { + ops, err := diffPermissions([]PermissionSpec{ + {Namespace: "compute/order", Name: "get"}, + {Namespace: "compute/order", Name: "legacy"}, + {Namespace: "compute/order", Name: "gone", Delete: true}, + }, current) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + t.Run("a server permission missing from the file fails the plan", func(t *testing.T) { + _, err := diffPermissions([]PermissionSpec{ + {Namespace: "compute/order", Name: "get"}, + }, current) + assert.ErrorContains(t, err, "compute/order:legacy") + assert.ErrorContains(t, err, "delete: true") + }) + + t.Run("conflicting delete flags for the same permission fail", func(t *testing.T) { + _, err := diffPermissions([]PermissionSpec{ + {Namespace: "compute/order", Name: "get"}, + {Namespace: "compute/order", Name: "get", Delete: true}, + {Namespace: "compute/order", Name: "legacy"}, + }, current) + assert.ErrorContains(t, err, "listed both with and without delete") + }) + + t.Run("distinct entries that flatten to the same slug fail", func(t *testing.T) { + _, err := diffPermissions([]PermissionSpec{ + {Namespace: "compute/order", Name: "get"}, + {Namespace: "compute/order", Name: "legacy"}, + {Namespace: "resource/order_item", Name: "get"}, + {Namespace: "resource_order/item", Name: "get"}, + }, current) + assert.ErrorContains(t, err, `collide on the same slug "resource_order_item_get"`) + }) + + t.Run("rejects base-schema namespaces and bad shapes", func(t *testing.T) { + for _, s := range []PermissionSpec{ + {Namespace: "app/organization", Name: "hack"}, + {Namespace: "app", Name: "hack"}, + {Namespace: "compute", Name: "get"}, + {Namespace: "compute/order", Name: "not-alnum"}, + {Namespace: "compute/order", Name: ""}, + } { + _, err := diffPermissions([]PermissionSpec{s}, nil) + assert.Error(t, err, "%+v", s) + } + }) +} diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go index 990626d1b..049f4bb3b 100644 --- a/internal/reconcile/reconcile.go +++ b/internal/reconcile/reconcile.go @@ -48,6 +48,13 @@ type parsedDocument struct { spec []byte } +// kindDependencies maps a kind to kinds whose objects it references. In one +// file, a dependency must not come after its dependent: that order would fail +// mid-apply instead of up front. +var kindDependencies = map[string][]string{ + KindRole: {KindPermission}, +} + // parseDocuments reads and checks every document in the file before anything // runs: the version must be known, the kind registered, and the spec present. // A missing spec marshals to "null"/"" (usually a typo); it is rejected rather @@ -83,6 +90,15 @@ func parseDocuments(registry map[string]Reconciler, data []byte) ([]parsedDocume } docs = append(docs, parsedDocument{kind: doc.Kind, spec: specBytes}) } + for i, doc := range docs { + for _, dep := range kindDependencies[doc.kind] { + for _, later := range docs[i+1:] { + if later.kind == dep { + return nil, fmt.Errorf("kind %q must come before kind %q in the file", dep, doc.kind) + } + } + } + } return docs, nil } diff --git a/internal/reconcile/role.go b/internal/reconcile/role.go new file mode 100644 index 000000000..6e42330e9 --- /dev/null +++ b/internal/reconcile/role.go @@ -0,0 +1,315 @@ +package reconcile + +import ( + "fmt" + "sort" + "strings" + + "github.com/raystack/frontier/core/permission" + "github.com/raystack/frontier/internal/bootstrap/schema" +) + +// KindRole is the desired-state document kind for platform-level roles. +const KindRole = "Role" + +const opUpdate opAction = "update" + +// RoleSpec is one desired platform-level role. Name is the identity and never +// changes. For custom roles, a field that is present is managed and a field +// that is omitted keeps its server value; permissions must be listed. +// Predefined roles converge to their shipped definition instead: an entry +// overrides the fields it lists, an omitted field takes the definition's +// value, and a predefined role absent from the file resets to the definition. +// Predefined roles cannot be deleted — bootstrap recreates a missing one on +// the next boot. +type RoleSpec struct { + Name string `yaml:"name"` + Title string `yaml:"title,omitempty"` + Description string `yaml:"description,omitempty"` + Permissions []string `yaml:"permissions,omitempty"` + Scopes []string `yaml:"scopes,omitempty"` + Delete bool `yaml:"delete,omitempty"` +} + +// currentRole is one platform role as returned by ListRoles. Description is a +// role metadata value, not a top-level field, so it is read from there. +type currentRole struct { + ID string + Name string + Title string + Description string + Permissions []string // slugs + Scopes []string +} + +// roleOp is a single planned change. For adds and updates, spec carries the +// final values to send (desired fields merged over current ones). +type roleOp struct { + action opAction + spec RoleSpec + id string // server role id, set for updates and deletes + detail string // which fields change, for the plan output +} + +func (o roleOp) String() string { + switch o.action { + case opRemove: + return fmt.Sprintf("delete role %s", o.spec.Name) + case opUpdate: + return fmt.Sprintf("update role %s (%s)", o.spec.Name, o.detail) + default: + return fmt.Sprintf("add role %s", o.spec.Name) + } +} + +// predefinedRole returns the shipped definition of a predefined role name. +func predefinedRole(name string) (schema.RoleDefinition, bool) { + for _, def := range schema.PredefinedRoles { + if def.Name == name { + return def, true + } + } + return schema.RoleDefinition{}, false +} + +// normalizePermissions maps permission references in any accepted form +// (slug, service/resource:verb, service.resource.verb) to slugs, deduped and +// sorted. +func normalizePermissions(refs []string) []string { + set := map[string]struct{}{} + for _, ref := range refs { + set[permission.ParsePermissionName(ref)] = struct{}{} + } + out := make([]string, 0, len(set)) + for slug := range set { + out = append(out, slug) + } + sort.Strings(out) + return out +} + +func sortedCopy(in []string) []string { + out := append([]string(nil), in...) + sort.Strings(out) + return out +} + +func stringSetsEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +// validateRoleSpec rejects entries the flow cannot manage. A managed-empty +// permission list is rejected for both role types: an exported file cannot +// represent it (empty lists are omitted), so it would not survive a round trip. +func validateRoleSpec(s RoleSpec, isPredefined bool) error { + if strings.TrimSpace(s.Name) == "" { + return fmt.Errorf("name is required") + } + if s.Delete { + if isPredefined { + return fmt.Errorf("cannot delete a predefined role; bootstrap recreates it on the next boot") + } + return nil + } + if isPredefined { + if s.Permissions != nil && len(s.Permissions) == 0 { + return fmt.Errorf("cannot set a predefined role's permissions to an empty list") + } + if s.Scopes != nil && len(s.Scopes) == 0 { + return fmt.Errorf("cannot set a predefined role's scopes to an empty list") + } + return nil + } + if len(s.Permissions) == 0 { + return fmt.Errorf("a custom role must list at least one permission") + } + return nil +} + +// diffRoles returns the ops that make the current platform roles match the +// desired spec. Custom roles are authoritative: every one on the server must +// appear in the spec — kept, or marked delete — so nothing is removed by +// omission. Predefined roles on the server converge to their shipped +// definition with the file entry, if any, laid over it; boot owns creating +// missing ones, so a predefined role absent from the server is not created +// here (and cannot be represented by an export anyway). +func diffRoles(desired []RoleSpec, current []currentRole) ([]roleOp, error) { + byName := make(map[string]currentRole, len(current)) + for _, c := range current { + byName[c.Name] = c + } + + seen := map[string]struct{}{} + overrides := map[string]RoleSpec{} + var customs []RoleSpec + for _, s := range desired { + _, isPredefined := predefinedRole(s.Name) + if err := validateRoleSpec(s, isPredefined); err != nil { + return nil, fmt.Errorf("invalid role spec %q: %w", s.Name, err) + } + if _, dup := seen[s.Name]; dup { + return nil, fmt.Errorf("role %q is listed more than once", s.Name) + } + seen[s.Name] = struct{}{} + if isPredefined { + overrides[s.Name] = s + } else { + customs = append(customs, s) + } + } + + var adds, updates, removes []roleOp + + // Predefined roles, in definition order. A duplicate definition keeps the + // first one, matching what boot creates. + seeded := map[string]struct{}{} + for _, def := range schema.PredefinedRoles { + if _, dup := seeded[def.Name]; dup { + continue + } + seeded[def.Name] = struct{}{} + cur, exists := byName[def.Name] + s, listed := overrides[def.Name] + if !exists { + if listed { + return nil, fmt.Errorf("predefined role %q not found on the server; bootstrap creates it at boot", def.Name) + } + continue + } + var entry *RoleSpec + if listed { + entry = &s + } + if op, changed := diffPredefinedRole(def, entry, cur); changed { + updates = append(updates, op) + } + } + + for _, s := range customs { + cur, exists := byName[s.Name] + if s.Delete { + if exists { + removes = append(removes, roleOp{action: opRemove, spec: s, id: cur.ID}) + } + continue + } + desiredPerms := normalizePermissions(s.Permissions) + if !exists { + adds = append(adds, roleOp{action: opAdd, spec: RoleSpec{ + Name: s.Name, + Title: s.Title, + Description: s.Description, + Permissions: desiredPerms, + Scopes: sortedCopy(s.Scopes), + }}) + continue + } + var changes []string + merged := RoleSpec{Name: s.Name, Title: cur.Title, Description: cur.Description, Permissions: sortedCopy(cur.Permissions), Scopes: sortedCopy(cur.Scopes)} + if s.Title != "" && s.Title != cur.Title { + merged.Title = s.Title + changes = append(changes, "title") + } + if s.Description != "" && s.Description != cur.Description { + merged.Description = s.Description + changes = append(changes, "description") + } + if !stringSetsEqual(desiredPerms, sortedCopy(cur.Permissions)) { + merged.Permissions = desiredPerms + changes = append(changes, "permissions") + } + if s.Scopes != nil && !stringSetsEqual(sortedCopy(s.Scopes), sortedCopy(cur.Scopes)) { + merged.Scopes = sortedCopy(s.Scopes) + changes = append(changes, "scopes") + } + if len(changes) > 0 { + updates = append(updates, roleOp{action: opUpdate, spec: merged, id: cur.ID, detail: strings.Join(changes, ", ")}) + } + } + + var unaccounted []string + for name := range byName { + if _, ok := seen[name]; ok { + continue + } + if _, isPredefined := predefinedRole(name); isPredefined { + continue + } + unaccounted = append(unaccounted, name) + } + if len(unaccounted) > 0 { + sort.Strings(unaccounted) + return nil, fmt.Errorf("roles exist on the server but are not in the file: %s; keep them or mark them 'delete: true'", strings.Join(unaccounted, ", ")) + } + + ops := append(adds, updates...) + return append(ops, removes...), nil +} + +// diffPredefinedRole compares one predefined role on the server against its +// shipped definition with the file entry, if any, laid over it. An omitted +// field converges to the definition, not the server value. A field whose +// server value is empty converges only when the entry lists it: an empty +// value cannot be written to or read back from a file, so converging it +// unlisted would break the export round trip. +func diffPredefinedRole(def schema.RoleDefinition, s *RoleSpec, cur currentRole) (roleOp, bool) { + merged := RoleSpec{Name: def.Name, Title: cur.Title, Description: cur.Description, Permissions: sortedCopy(cur.Permissions), Scopes: sortedCopy(cur.Scopes)} + var changes []string + + title := def.Title + titleListed := s != nil && s.Title != "" + if titleListed { + title = s.Title + } + if title != cur.Title && (titleListed || cur.Title != "") { + merged.Title = title + changes = append(changes, "title") + } + + description := def.Description + descriptionListed := s != nil && s.Description != "" + if descriptionListed { + description = s.Description + } + if description != cur.Description && (descriptionListed || cur.Description != "") { + merged.Description = description + changes = append(changes, "description") + } + + perms := normalizePermissions(def.Permissions) + permsListed := s != nil && s.Permissions != nil + if permsListed { + perms = normalizePermissions(s.Permissions) + } + if !stringSetsEqual(perms, sortedCopy(cur.Permissions)) && (permsListed || len(cur.Permissions) > 0) { + merged.Permissions = perms + changes = append(changes, "permissions") + } + + scopes := sortedCopy(def.Scopes) + scopesListed := s != nil && s.Scopes != nil + if scopesListed { + scopes = sortedCopy(s.Scopes) + } + if !stringSetsEqual(scopes, sortedCopy(cur.Scopes)) && (scopesListed || len(cur.Scopes) > 0) { + merged.Scopes = scopes + changes = append(changes, "scopes") + } + + if len(changes) == 0 { + return roleOp{}, false + } + detail := strings.Join(changes, ", ") + if s == nil { + detail += "; not in file, reset to default" + } + return roleOp{action: opUpdate, spec: merged, id: cur.ID, detail: detail}, true +} diff --git a/internal/reconcile/role_reconciler.go b/internal/reconcile/role_reconciler.go new file mode 100644 index 000000000..f26a173b8 --- /dev/null +++ b/internal/reconcile/role_reconciler.go @@ -0,0 +1,199 @@ +package reconcile + +import ( + "context" + "fmt" + "sort" + + "connectrpc.com/connect" + frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" + "google.golang.org/protobuf/types/known/structpb" + "gopkg.in/yaml.v3" +) + +// Roles the reconciler creates or updates are stamped with this metadata, so +// their origin is visible and other tooling can tell them apart. +const ( + managedByKey = "managed_by" + managedByValue = "frontier-reconcile" + descriptionKey = "description" +) + +// RoleAPI is the API subset the role reconciler needs. Reads live on +// FrontierService, writes on AdminService; the caller provides one value that +// serves both. +type RoleAPI interface { + ListRoles(context.Context, *connect.Request[frontierv1beta1.ListRolesRequest]) (*connect.Response[frontierv1beta1.ListRolesResponse], error) + CreateRole(context.Context, *connect.Request[frontierv1beta1.CreateRoleRequest]) (*connect.Response[frontierv1beta1.CreateRoleResponse], error) + UpdateRole(context.Context, *connect.Request[frontierv1beta1.UpdateRoleRequest]) (*connect.Response[frontierv1beta1.UpdateRoleResponse], error) + DeleteRole(context.Context, *connect.Request[frontierv1beta1.DeleteRoleRequest]) (*connect.Response[frontierv1beta1.DeleteRoleResponse], error) +} + +// RoleReconciler makes platform-level roles match the desired spec. The role +// name is the identity; title, permissions, and scopes are the managed fields. +type RoleReconciler struct { + client RoleAPI + header string +} + +func NewRoleReconciler(client RoleAPI, header string) *RoleReconciler { + return &RoleReconciler{client: client, header: header} +} + +func (r *RoleReconciler) Kind() string { return KindRole } + +func (r *RoleReconciler) Reconcile(ctx context.Context, spec []byte, dryRun bool) (Report, error) { + var specs []RoleSpec + if err := yaml.Unmarshal(spec, &specs); err != nil { + return Report{}, fmt.Errorf("parse %s spec: %w", KindRole, err) + } + + current, err := r.fetchCurrent(ctx) + if err != nil { + return Report{}, err + } + + ops, err := diffRoles(specs, current) + if err != nil { + return Report{}, err + } + + rep := Report{Kind: KindRole, 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 +} + +// Export returns the current platform roles as a desired-state spec. Custom +// roles are exported in full. Predefined roles are exported only when they +// differ from their shipped definition (title, permissions, or scopes), and +// only with the differing fields, so converged ones stay out of the file. +// The comparisons mirror diffPredefinedRole exactly — including skipping +// fields that are empty on the server, which a file cannot represent — so +// reconciling an export's output always plans no changes. +func (r *RoleReconciler) Export(ctx context.Context) (any, error) { + current, err := r.fetchCurrent(ctx) + if err != nil { + return nil, err + } + sort.Slice(current, func(i, j int) bool { return current[i].Name < current[j].Name }) + + specs := make([]RoleSpec, 0, len(current)) + for _, c := range current { + def, isPredefined := predefinedRole(c.Name) + if !isPredefined { + specs = append(specs, RoleSpec{ + Name: c.Name, + Title: c.Title, + Description: c.Description, + Permissions: sortedCopy(c.Permissions), + Scopes: sortedCopy(c.Scopes), + }) + continue + } + entry := RoleSpec{Name: c.Name} + changed := false + if c.Title != "" && c.Title != def.Title { + entry.Title = c.Title + changed = true + } + if c.Description != "" && c.Description != def.Description { + entry.Description = c.Description + changed = true + } + if len(c.Permissions) > 0 && !stringSetsEqual(normalizePermissions(c.Permissions), normalizePermissions(def.Permissions)) { + entry.Permissions = sortedCopy(c.Permissions) + changed = true + } + if len(c.Scopes) > 0 && !stringSetsEqual(sortedCopy(c.Scopes), sortedCopy(def.Scopes)) { + entry.Scopes = sortedCopy(c.Scopes) + changed = true + } + if changed { + specs = append(specs, entry) + } + } + return specs, nil +} + +func (r *RoleReconciler) fetchCurrent(ctx context.Context) ([]currentRole, error) { + resp, err := r.client.ListRoles(ctx, authReq(&frontierv1beta1.ListRolesRequest{}, r.header)) + if err != nil { + return nil, fmt.Errorf("list roles: %w", err) + } + var current []currentRole + for _, ro := range resp.Msg.GetRoles() { + current = append(current, currentRole{ + ID: ro.GetId(), + Name: ro.GetName(), + Title: ro.GetTitle(), + Description: metadataString(ro.GetMetadata(), descriptionKey), + Permissions: ro.GetPermissions(), + Scopes: ro.GetScopes(), + }) + } + return current, nil +} + +// metadataString reads a string value from role metadata, or "" if absent. +func metadataString(md *structpb.Struct, key string) string { + if md == nil { + return "" + } + if v, ok := md.GetFields()[key]; ok { + return v.GetStringValue() + } + return "" +} + +func (r *RoleReconciler) apply(ctx context.Context, op roleOp) error { + switch op.action { + case opAdd: + body, err := roleBody(op.spec) + if err != nil { + return err + } + _, err = r.client.CreateRole(ctx, authReq(&frontierv1beta1.CreateRoleRequest{Body: body}, r.header)) + return err + case opUpdate: + body, err := roleBody(op.spec) + if err != nil { + return err + } + _, err = r.client.UpdateRole(ctx, authReq(&frontierv1beta1.UpdateRoleRequest{Id: op.id, Body: body}, r.header)) + return err + case opRemove: + _, err := r.client.DeleteRole(ctx, authReq(&frontierv1beta1.DeleteRoleRequest{Id: op.id}, r.header)) + return err + default: + return fmt.Errorf("unknown op action %q", op.action) + } +} + +func roleBody(spec RoleSpec) (*frontierv1beta1.RoleRequestBody, error) { + fields := map[string]any{managedByKey: managedByValue} + if spec.Description != "" { + fields[descriptionKey] = spec.Description + } + md, err := structpb.NewStruct(fields) + if err != nil { + return nil, fmt.Errorf("build role metadata: %w", err) + } + return &frontierv1beta1.RoleRequestBody{ + Name: spec.Name, + Title: spec.Title, + Permissions: spec.Permissions, + Scopes: spec.Scopes, + Metadata: md, + }, nil +} diff --git a/internal/reconcile/role_reconciler_test.go b/internal/reconcile/role_reconciler_test.go new file mode 100644 index 000000000..b7cf87254 --- /dev/null +++ b/internal/reconcile/role_reconciler_test.go @@ -0,0 +1,285 @@ +package reconcile + +import ( + "context" + "errors" + "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 fakeRoleAPI struct { + roles []*frontierv1beta1.Role + created []*frontierv1beta1.RoleRequestBody + updated map[string]*frontierv1beta1.RoleRequestBody + deleted []string + createErr error + updateErr error +} + +func (f *fakeRoleAPI) ListRoles(_ context.Context, _ *connect.Request[frontierv1beta1.ListRolesRequest]) (*connect.Response[frontierv1beta1.ListRolesResponse], error) { + return connect.NewResponse(&frontierv1beta1.ListRolesResponse{Roles: f.roles}), nil +} + +func (f *fakeRoleAPI) CreateRole(_ context.Context, req *connect.Request[frontierv1beta1.CreateRoleRequest]) (*connect.Response[frontierv1beta1.CreateRoleResponse], error) { + if f.createErr != nil { + return nil, f.createErr + } + f.created = append(f.created, req.Msg.GetBody()) + return connect.NewResponse(&frontierv1beta1.CreateRoleResponse{}), nil +} + +func (f *fakeRoleAPI) UpdateRole(_ context.Context, req *connect.Request[frontierv1beta1.UpdateRoleRequest]) (*connect.Response[frontierv1beta1.UpdateRoleResponse], error) { + if f.updateErr != nil { + return nil, f.updateErr + } + if f.updated == nil { + f.updated = map[string]*frontierv1beta1.RoleRequestBody{} + } + f.updated[req.Msg.GetId()] = req.Msg.GetBody() + return connect.NewResponse(&frontierv1beta1.UpdateRoleResponse{}), nil +} + +func (f *fakeRoleAPI) DeleteRole(_ context.Context, req *connect.Request[frontierv1beta1.DeleteRoleRequest]) (*connect.Response[frontierv1beta1.DeleteRoleResponse], error) { + f.deleted = append(f.deleted, req.Msg.GetId()) + return connect.NewResponse(&frontierv1beta1.DeleteRoleResponse{}), nil +} + +func rolePB(id, name, title string, permissions []string, scopes ...string) *frontierv1beta1.Role { + return &frontierv1beta1.Role{Id: id, Name: name, Title: title, Permissions: permissions, Scopes: scopes} +} + +func rolePBWithDesc(id, name, title, description string, permissions []string, scopes ...string) *frontierv1beta1.Role { + r := rolePB(id, name, title, permissions, scopes...) + md, _ := structpb.NewStruct(map[string]any{descriptionKey: description}) + r.Metadata = md + return r +} + +func ownerDefaultPB() *frontierv1beta1.Role { + return rolePB("r-owner", schema.RoleOrganizationOwner, "Owner", + []string{"app_organization_administer"}, schema.OrganizationNamespace) +} + +func TestRoleReconciler(t *testing.T) { + t.Run("applies add, update, and delete with the managed-by marker", func(t *testing.T) { + api := &fakeRoleAPI{roles: []*frontierv1beta1.Role{ + ownerDefaultPB(), + rolePB("r1", "compute_manager", "Compute Manager", []string{"compute_order_get"}), + rolePB("r2", "old_role", "Old", []string{"compute_order_get"}), + }} + spec := []byte(` +- {name: compute_manager, title: Compute Admin, permissions: [compute_order_get]} +- {name: old_role, delete: true} +- {name: new_role, title: New, permissions: [compute_order_get]} +`) + rep, err := NewRoleReconciler(api, "").Reconcile(context.Background(), spec, false) + + assert.NoError(t, err) + assert.Equal(t, 3, rep.Applied) + if assert.Len(t, api.created, 1) { + assert.Equal(t, "new_role", api.created[0].GetName()) + assert.Equal(t, managedByValue, api.created[0].GetMetadata().GetFields()[managedByKey].GetStringValue()) + } + if assert.Len(t, api.updated, 1) { + body := api.updated["r1"] + if assert.NotNil(t, body) { + assert.Equal(t, "compute_manager", body.GetName()) // identity never changes + assert.Equal(t, "Compute Admin", body.GetTitle()) + assert.Equal(t, []string{"compute_order_get"}, body.GetPermissions()) + assert.Equal(t, managedByValue, body.GetMetadata().GetFields()[managedByKey].GetStringValue()) + } + } + assert.Equal(t, []string{"r2"}, api.deleted) + }) + + t.Run("updates a predefined role's title and permissions by id", func(t *testing.T) { + api := &fakeRoleAPI{roles: []*frontierv1beta1.Role{ownerDefaultPB()}} + spec := []byte(` +- name: app_organization_owner + title: Workspace Owner + permissions: [app_organization_administer, app_organization_get] +`) + rep, err := NewRoleReconciler(api, "").Reconcile(context.Background(), spec, false) + + assert.NoError(t, err) + assert.Equal(t, 1, rep.Applied) + body := api.updated["r-owner"] + if assert.NotNil(t, body) { + assert.Equal(t, schema.RoleOrganizationOwner, body.GetName()) + assert.Equal(t, "Workspace Owner", body.GetTitle()) + assert.ElementsMatch(t, []string{"app_organization_administer", "app_organization_get"}, body.GetPermissions()) + assert.Equal(t, []string{schema.OrganizationNamespace}, body.GetScopes()) // omitted in the entry: the definition's value + } + }) + + t.Run("an unlisted drifted predefined role resets to its definition", func(t *testing.T) { + api := &fakeRoleAPI{roles: []*frontierv1beta1.Role{ + rolePB("r-owner", schema.RoleOrganizationOwner, "Renamed Owner", + []string{"app_organization_administer", "app_organization_get"}, schema.OrganizationNamespace), + }} + + rep, err := NewRoleReconciler(api, "").Reconcile(context.Background(), []byte("[]\n"), false) + + assert.NoError(t, err) + assert.Equal(t, []string{"update role app_organization_owner (title, permissions; not in file, reset to default)"}, rep.Planned) + assert.Equal(t, 1, rep.Applied) + body := api.updated["r-owner"] + if assert.NotNil(t, body) { + assert.Equal(t, "Owner", body.GetTitle()) + assert.Equal(t, []string{"app_organization_administer"}, body.GetPermissions()) + } + }) + + t.Run("a predefined role with empty legacy fields round-trips as converged", func(t *testing.T) { + // A row from before the scopes column: empty on the server, non-empty in + // the definition. A file cannot hold an empty value, so export omits the + // role and reconcile leaves the field alone instead of fighting forever. + api := &fakeRoleAPI{roles: []*frontierv1beta1.Role{ + rolePB("r-owner", schema.RoleOrganizationOwner, "Owner", []string{"app_organization_administer"}), + }} + registry := map[string]Reconciler{KindRole: NewRoleReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindRole) + assert.NoError(t, err) + assert.Contains(t, string(out), "spec: []") + + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + + t.Run("applying a role with a description writes it to metadata", func(t *testing.T) { + api := &fakeRoleAPI{} + spec := []byte("- {name: new_role, description: Runs compute orders, permissions: [compute_order_get]}\n") + + rep, err := NewRoleReconciler(api, "").Reconcile(context.Background(), spec, false) + + assert.NoError(t, err) + assert.Equal(t, 1, rep.Applied) + if assert.Len(t, api.created, 1) { + md := api.created[0].GetMetadata().GetFields() + assert.Equal(t, "Runs compute orders", md[descriptionKey].GetStringValue()) + assert.Equal(t, managedByValue, md[managedByKey].GetStringValue()) + } + }) + + t.Run("a custom role's description round-trips", func(t *testing.T) { + api := &fakeRoleAPI{roles: []*frontierv1beta1.Role{ + ownerDefaultPB(), + rolePBWithDesc("r1", "compute_manager", "Compute Manager", "Runs compute orders", []string{"compute_order_get"}), + }} + registry := map[string]Reconciler{KindRole: NewRoleReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindRole) + assert.NoError(t, err) + assert.Contains(t, string(out), "Runs compute orders") + + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + + t.Run("reconciling an exported document plans no changes", func(t *testing.T) { + api := &fakeRoleAPI{roles: []*frontierv1beta1.Role{ + ownerDefaultPB(), // matches defaults: not exported, skipped when unlisted + rolePB("r-viewer", "app_organization_viewer", "Everyone", []string{"app_organization_get"}), // retitled predefined + rolePB("r1", "compute_manager", "Compute Manager", []string{"compute_order_update", "compute_order_get"}, "compute/order"), + }} + registry := map[string]Reconciler{KindRole: NewRoleReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindRole) + assert.NoError(t, err) + assert.NotContains(t, string(out), schema.RoleOrganizationOwner) + assert.Contains(t, string(out), "Everyone") + + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + + t.Run("a role referencing a missing permission fails at the API call", func(t *testing.T) { + // Kinds are independent: the reconciler does not resolve permission + // references itself. The server rejects them, and that failure must + // surface with the failing op named and the run stopped. + api := &fakeRoleAPI{ + createErr: connect.NewError(connect.CodeInvalidArgument, + errors.New("compute_order_missing: permission doesn't exist")), + } + spec := []byte("- {name: new_role, permissions: [compute_order_missing]}\n") + + rep, err := NewRoleReconciler(api, "").Reconcile(context.Background(), spec, false) + + assert.ErrorContains(t, err, "apply [add role new_role]") + assert.ErrorContains(t, err, "permission doesn't exist") + assert.Zero(t, rep.Applied) + assert.Equal(t, []string{"add role new_role"}, rep.Planned) // the plan survives for the report + }) + + t.Run("export includes a predefined role whose permissions were narrowed", func(t *testing.T) { + // default title, changed permission set: exactly the review scenario. + api := &fakeRoleAPI{roles: []*frontierv1beta1.Role{ + rolePB("r-owner", schema.RoleOrganizationOwner, "Owner", + []string{"app_organization_get", "app_organization_update", "app_organization_policymanage"}, + schema.OrganizationNamespace), + }} + registry := map[string]Reconciler{KindRole: NewRoleReconciler(api, "")} + + spec, err := NewRoleReconciler(api, "").Export(context.Background()) + assert.NoError(t, err) + assert.Equal(t, []RoleSpec{{ + Name: schema.RoleOrganizationOwner, + Permissions: []string{"app_organization_get", "app_organization_policymanage", "app_organization_update"}, + }}, spec) + + out, err := Export(context.Background(), registry, KindRole) + assert.NoError(t, err) + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + + t.Run("export includes a predefined role whose scopes changed", func(t *testing.T) { + api := &fakeRoleAPI{roles: []*frontierv1beta1.Role{ + rolePB("r-owner", schema.RoleOrganizationOwner, "Owner", + []string{"app_organization_administer"}, schema.OrganizationNamespace, "compute/order"), + }} + registry := map[string]Reconciler{KindRole: NewRoleReconciler(api, "")} + + spec, err := NewRoleReconciler(api, "").Export(context.Background()) + assert.NoError(t, err) + assert.Equal(t, []RoleSpec{{ + Name: schema.RoleOrganizationOwner, + Scopes: []string{schema.OrganizationNamespace, "compute/order"}, + }}, spec) + + out, err := Export(context.Background(), registry, KindRole) + assert.NoError(t, err) + reports, err := Run(context.Background(), registry, out, true) + assert.NoError(t, err) + if assert.Len(t, reports, 1) { + assert.Empty(t, reports[0].Planned) + } + }) + + t.Run("export with only default predefined roles yields an empty list", func(t *testing.T) { + api := &fakeRoleAPI{roles: []*frontierv1beta1.Role{ownerDefaultPB()}} + registry := map[string]Reconciler{KindRole: NewRoleReconciler(api, "")} + + out, err := Export(context.Background(), registry, KindRole) + assert.NoError(t, err) + assert.Contains(t, string(out), "spec: []") + }) +} diff --git a/internal/reconcile/role_test.go b/internal/reconcile/role_test.go new file mode 100644 index 000000000..d487e95bf --- /dev/null +++ b/internal/reconcile/role_test.go @@ -0,0 +1,246 @@ +package reconcile + +import ( + "context" + "testing" + + "github.com/raystack/frontier/internal/bootstrap/schema" + "github.com/stretchr/testify/assert" +) + +func TestDiffRoles(t *testing.T) { + current := []currentRole{ + {ID: "r1", Name: "compute_manager", Title: "Compute Manager", + Permissions: []string{"compute_order_get", "compute_order_update"}, + Scopes: []string{"compute/order"}}, + {ID: "r2", Name: "old_role", Title: "Old", Permissions: []string{"compute_order_get"}}, + {ID: "r3", Name: schema.RoleOrganizationOwner, Title: "Owner", + Permissions: []string{"app_organization_administer"}, + Scopes: []string{schema.OrganizationNamespace}}, + } + + keepCustom := []RoleSpec{ + {Name: "compute_manager", Permissions: []string{"compute_order_get", "compute_order_update"}}, + {Name: "old_role", Permissions: []string{"compute_order_get"}}, + } + + t.Run("no changes when converged, unlisted predefined roles at defaults", func(t *testing.T) { + ops, err := diffRoles(keepCustom, current) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + t.Run("an unlisted predefined role that drifted resets to its definition", func(t *testing.T) { + drifted := append([]currentRole{}, current[:2]...) + drifted = append(drifted, currentRole{ID: "r3", Name: schema.RoleOrganizationOwner, Title: "Renamed Owner", + Permissions: []string{"app_organization_administer", "app_organization_get"}, + Scopes: []string{schema.OrganizationNamespace}}) + + ops, err := diffRoles(keepCustom, drifted) + + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, "update role app_organization_owner (title, permissions; not in file, reset to default)", ops[0].String()) + assert.Equal(t, "r3", ops[0].id) + assert.Equal(t, "Owner", ops[0].spec.Title) + assert.Equal(t, []string{"app_organization_administer"}, ops[0].spec.Permissions) + } + }) + + t.Run("a listed predefined role resets the fields it omits to the definition", func(t *testing.T) { + drifted := append([]currentRole{}, current[:2]...) + drifted = append(drifted, currentRole{ID: "r3", Name: schema.RoleOrganizationOwner, Title: "Renamed Owner", + Permissions: []string{"app_organization_administer"}, + Scopes: []string{schema.OrganizationNamespace}}) + + ops, err := diffRoles(append(keepCustom, RoleSpec{ + Name: schema.RoleOrganizationOwner, + Permissions: []string{"app_organization_administer", "app_organization_get"}, + }), drifted) + + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, "update role app_organization_owner (title, permissions)", ops[0].String()) + assert.Equal(t, "Owner", ops[0].spec.Title) // omitted in the entry: back to the definition, not the server value + assert.ElementsMatch(t, []string{"app_organization_administer", "app_organization_get"}, ops[0].spec.Permissions) + } + }) + + t.Run("empty server values converge only when listed", func(t *testing.T) { + legacy := append([]currentRole{}, current[:2]...) + legacy = append(legacy, currentRole{ID: "r3", Name: schema.RoleOrganizationOwner, Title: "Owner", + Permissions: []string{"app_organization_administer"}}) // scopes never recorded on this row + + ops, err := diffRoles(keepCustom, legacy) + assert.NoError(t, err) + assert.Empty(t, ops) // unlisted: an empty value cannot round-trip through a file, so it is settled + + ops, err = diffRoles(append(keepCustom, RoleSpec{ + Name: schema.RoleOrganizationOwner, Scopes: []string{schema.OrganizationNamespace}, + }), legacy) + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, "update role app_organization_owner (scopes)", ops[0].String()) + } + }) + + t.Run("permission references in any form match slugs", func(t *testing.T) { + ops, err := diffRoles([]RoleSpec{ + {Name: "compute_manager", Permissions: []string{"compute/order:get", "compute.order.update"}}, + {Name: "old_role", Permissions: []string{"compute_order_get"}}, + }, current) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + t.Run("adds, updates, and deletes in that order", func(t *testing.T) { + ops, err := diffRoles([]RoleSpec{ + {Name: "compute_manager", Title: "Compute Admin", + Permissions: []string{"compute_order_get", "compute_order_update"}}, + {Name: "old_role", Delete: true}, + {Name: "new_role", Title: "New", Permissions: []string{"compute_order_get"}}, + }, current) + + assert.NoError(t, err) + if assert.Len(t, ops, 3) { + assert.Equal(t, "add role new_role", ops[0].String()) + assert.Equal(t, "update role compute_manager (title)", ops[1].String()) + assert.Equal(t, "delete role old_role", ops[2].String()) + assert.Equal(t, "r2", ops[2].id) + } + }) + + t.Run("update merges managed fields over current values", func(t *testing.T) { + ops, err := diffRoles([]RoleSpec{ + {Name: "compute_manager", Permissions: []string{"compute_order_get"}}, // narrow the set, no title in spec + {Name: "old_role", Permissions: []string{"compute_order_get"}}, + }, current) + + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + op := ops[0] + assert.Equal(t, opUpdate, op.action) + assert.Equal(t, "permissions", op.detail) + assert.Equal(t, "Compute Manager", op.spec.Title) // kept from server + assert.Equal(t, []string{"compute_order_get"}, op.spec.Permissions) + assert.Equal(t, []string{"compute/order"}, op.spec.Scopes) // kept from server + } + }) + + t.Run("a custom role's description is managed like its title", func(t *testing.T) { + ops, err := diffRoles([]RoleSpec{ + {Name: "compute_manager", Description: "Runs compute orders", + Permissions: []string{"compute_order_get", "compute_order_update"}}, + {Name: "old_role", Permissions: []string{"compute_order_get"}}, + }, current) + + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, "update role compute_manager (description)", ops[0].String()) + assert.Equal(t, "Runs compute orders", ops[0].spec.Description) + } + }) + + t.Run("an unlisted predefined role's drifted description resets to the definition", func(t *testing.T) { + drifted := append([]currentRole{}, current[:2]...) + drifted = append(drifted, currentRole{ID: "r3", Name: schema.RoleOrganizationOwner, Title: "Owner", + Description: "hand-edited note", + Permissions: []string{"app_organization_administer"}, + Scopes: []string{schema.OrganizationNamespace}}) + + ops, err := diffRoles(keepCustom, drifted) + + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, "update role app_organization_owner (description; not in file, reset to default)", ops[0].String()) + assert.Equal(t, "", ops[0].spec.Description) // the definition sets no description + } + }) + + t.Run("predefined role title and permissions can be managed", func(t *testing.T) { + specs := append(keepCustom, RoleSpec{ + Name: schema.RoleOrganizationOwner, + Title: "Workspace Owner", + Permissions: []string{"app_organization_administer", "app_organization_get"}, + }) + ops, err := diffRoles(specs, current) + + assert.NoError(t, err) + if assert.Len(t, ops, 1) { + assert.Equal(t, "update role app_organization_owner (title, permissions)", ops[0].String()) + assert.Equal(t, "r3", ops[0].id) + } + }) + + t.Run("deleting a predefined role is rejected", func(t *testing.T) { + _, err := diffRoles(append(keepCustom, RoleSpec{Name: schema.RoleOrganizationOwner, Delete: true}), current) + assert.ErrorContains(t, err, "predefined role") + }) + + t.Run("a listed predefined role missing on the server fails the plan", func(t *testing.T) { + _, err := diffRoles(append(keepCustom, RoleSpec{Name: schema.GroupOwnerRole, Title: "X"}), current) + assert.ErrorContains(t, err, "not found on the server") + }) + + t.Run("a custom server role missing from the file fails the plan", func(t *testing.T) { + _, err := diffRoles([]RoleSpec{ + {Name: "compute_manager", Permissions: []string{"compute_order_get", "compute_order_update"}}, + }, current) + assert.ErrorContains(t, err, "old_role") + assert.ErrorContains(t, err, "delete: true") + }) + + t.Run("a custom role must list at least one permission", func(t *testing.T) { + _, err := diffRoles([]RoleSpec{{Name: "compute_manager", Title: "X"}}, current) + assert.ErrorContains(t, err, "must list at least one permission") + + _, err = diffRoles([]RoleSpec{{Name: "compute_manager", Permissions: []string{}}}, current) + assert.ErrorContains(t, err, "must list at least one permission") + }) + + t.Run("emptying a predefined role's permissions or scopes is rejected", func(t *testing.T) { + _, err := diffRoles(append(keepCustom, RoleSpec{ + Name: schema.RoleOrganizationOwner, Permissions: []string{}, + }), current) + assert.ErrorContains(t, err, "permissions to an empty list") + + _, err = diffRoles(append(keepCustom, RoleSpec{ + Name: schema.RoleOrganizationOwner, Scopes: []string{}, + }), current) + assert.ErrorContains(t, err, "scopes to an empty list") + }) + + t.Run("duplicate names fail", func(t *testing.T) { + _, err := diffRoles([]RoleSpec{ + {Name: "compute_manager", Permissions: []string{"a"}}, + {Name: "compute_manager", Permissions: []string{"b"}}, + }, current) + assert.ErrorContains(t, err, "listed more than once") + }) + + t.Run("delete of an absent custom role is a no-op", func(t *testing.T) { + ops, err := diffRoles(append(keepCustom, RoleSpec{Name: "already_gone", Delete: true}), current) + assert.NoError(t, err) + assert.Empty(t, ops) + }) +} + +func TestRun_RoleBeforePermissionFailsFast(t *testing.T) { + roleAPI := &fakeRoleAPI{} + permAPI := &fakePermissionAPI{} + registry := map[string]Reconciler{ + KindRole: NewRoleReconciler(roleAPI, ""), + KindPermission: NewPermissionReconciler(permAPI, ""), + } + data := []byte("kind: Role\nspec: []\n---\nkind: Permission\nspec: []\n") + + _, err := Run(context.Background(), registry, data, false) + + assert.ErrorContains(t, err, `kind "Permission" must come before kind "Role"`) + assert.Empty(t, roleAPI.created) // nothing dispatched + assert.Empty(t, permAPI.created) + + // the right order passes the check + _, err = Run(context.Background(), registry, []byte("kind: Permission\nspec: []\n---\nkind: Role\nspec: []\n"), true) + assert.NoError(t, err) +} From 98d86e0665dbe3e8509b3e9cb2a5e9cef05297e5 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Thu, 16 Jul 2026 14:19:57 +0530 Subject: [PATCH 2/3] fix(reconcile): reject unknown spec fields and keep unmanaged role metadata on update --- internal/reconcile/permission_reconciler.go | 3 +- .../reconcile/permission_reconciler_test.go | 11 +++++++ internal/reconcile/reconcile.go | 12 ++++++++ internal/reconcile/role.go | 19 +++++++----- internal/reconcile/role_reconciler.go | 26 ++++++++++++---- internal/reconcile/role_reconciler_test.go | 30 +++++++++++++++++++ 6 files changed, 86 insertions(+), 15 deletions(-) diff --git a/internal/reconcile/permission_reconciler.go b/internal/reconcile/permission_reconciler.go index c35c72508..9600917d1 100644 --- a/internal/reconcile/permission_reconciler.go +++ b/internal/reconcile/permission_reconciler.go @@ -8,7 +8,6 @@ import ( "connectrpc.com/connect" "github.com/raystack/frontier/internal/bootstrap/schema" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" - "gopkg.in/yaml.v3" ) // PermissionAPI is the API subset the permission reconciler needs. Reads live @@ -35,7 +34,7 @@ func (r *PermissionReconciler) Kind() string { return KindPermission } func (r *PermissionReconciler) Reconcile(ctx context.Context, spec []byte, dryRun bool) (Report, error) { var specs []PermissionSpec - if err := yaml.Unmarshal(spec, &specs); err != nil { + if err := decodeSpec(spec, &specs); err != nil { return Report{}, fmt.Errorf("parse %s spec: %w", KindPermission, err) } diff --git a/internal/reconcile/permission_reconciler_test.go b/internal/reconcile/permission_reconciler_test.go index 121d70dce..1724b5d01 100644 --- a/internal/reconcile/permission_reconciler_test.go +++ b/internal/reconcile/permission_reconciler_test.go @@ -66,6 +66,17 @@ func TestPermissionReconciler(t *testing.T) { assert.Empty(t, api.created) }) + t.Run("an unknown field in the spec fails the plan", func(t *testing.T) { + api := &fakePermissionAPI{} + // `delet` instead of `delete`: must fail, not silently ignore the delete + spec := []byte("- {namespace: compute/order, name: get, delet: true}\n") + + _, err := NewPermissionReconciler(api, "").Reconcile(context.Background(), spec, true) + + assert.ErrorContains(t, err, "parse Permission spec") + assert.Empty(t, api.created) + }) + t.Run("reconciling an exported document plans no changes", func(t *testing.T) { api := &fakePermissionAPI{perms: []*frontierv1beta1.Permission{ permissionPB("b1", "app/project", "get"), diff --git a/internal/reconcile/reconcile.go b/internal/reconcile/reconcile.go index 049f4bb3b..d2f1179e1 100644 --- a/internal/reconcile/reconcile.go +++ b/internal/reconcile/reconcile.go @@ -102,6 +102,18 @@ func parseDocuments(registry map[string]Reconciler, data []byte) ([]parsedDocume return docs, nil } +// decodeSpec unmarshals a kind's spec with unknown fields rejected, so a typo +// in a field name (like `delet: true` for `delete`) fails the plan instead of +// being silently ignored, which would make a run quietly do the wrong thing. +func decodeSpec(spec []byte, out any) error { + dec := yaml.NewDecoder(bytes.NewReader(spec)) + dec.KnownFields(true) + if err := dec.Decode(out); err != nil && err != io.EOF { + return err + } + return nil +} + // Run applies a (possibly multi-document) desired-state file. The whole file is // parsed and checked first, so a malformed later document stops the run before // anything applies. Documents then dispatch in file order — dependency order is diff --git a/internal/reconcile/role.go b/internal/reconcile/role.go index 6e42330e9..82689bbd9 100644 --- a/internal/reconcile/role.go +++ b/internal/reconcile/role.go @@ -33,6 +33,8 @@ type RoleSpec struct { // currentRole is one platform role as returned by ListRoles. Description is a // role metadata value, not a top-level field, so it is read from there. +// Metadata holds the role's full current metadata, so an update can merge the +// reconciler-managed keys over it instead of dropping everything else. type currentRole struct { ID string Name string @@ -40,15 +42,18 @@ type currentRole struct { Description string Permissions []string // slugs Scopes []string + Metadata map[string]any } // roleOp is a single planned change. For adds and updates, spec carries the -// final values to send (desired fields merged over current ones). +// final values to send (desired fields merged over current ones). baseMetadata +// is the role's current metadata for an update, so managed keys merge over it. type roleOp struct { - action opAction - spec RoleSpec - id string // server role id, set for updates and deletes - detail string // which fields change, for the plan output + action opAction + spec RoleSpec + id string // server role id, set for updates and deletes + detail string // which fields change, for the plan output + baseMetadata map[string]any // current metadata to merge onto, set for updates } func (o roleOp) String() string { @@ -231,7 +236,7 @@ func diffRoles(desired []RoleSpec, current []currentRole) ([]roleOp, error) { changes = append(changes, "scopes") } if len(changes) > 0 { - updates = append(updates, roleOp{action: opUpdate, spec: merged, id: cur.ID, detail: strings.Join(changes, ", ")}) + updates = append(updates, roleOp{action: opUpdate, spec: merged, id: cur.ID, detail: strings.Join(changes, ", "), baseMetadata: cur.Metadata}) } } @@ -311,5 +316,5 @@ func diffPredefinedRole(def schema.RoleDefinition, s *RoleSpec, cur currentRole) if s == nil { detail += "; not in file, reset to default" } - return roleOp{action: opUpdate, spec: merged, id: cur.ID, detail: detail}, true + return roleOp{action: opUpdate, spec: merged, id: cur.ID, detail: detail, baseMetadata: cur.Metadata}, true } diff --git a/internal/reconcile/role_reconciler.go b/internal/reconcile/role_reconciler.go index f26a173b8..1ef717d4d 100644 --- a/internal/reconcile/role_reconciler.go +++ b/internal/reconcile/role_reconciler.go @@ -8,7 +8,6 @@ import ( "connectrpc.com/connect" frontierv1beta1 "github.com/raystack/frontier/proto/v1beta1" "google.golang.org/protobuf/types/known/structpb" - "gopkg.in/yaml.v3" ) // Roles the reconciler creates or updates are stamped with this metadata, so @@ -44,7 +43,7 @@ func (r *RoleReconciler) Kind() string { return KindRole } func (r *RoleReconciler) Reconcile(ctx context.Context, spec []byte, dryRun bool) (Report, error) { var specs []RoleSpec - if err := yaml.Unmarshal(spec, &specs); err != nil { + if err := decodeSpec(spec, &specs); err != nil { return Report{}, fmt.Errorf("parse %s spec: %w", KindRole, err) } @@ -133,6 +132,10 @@ func (r *RoleReconciler) fetchCurrent(ctx context.Context) ([]currentRole, error } var current []currentRole for _, ro := range resp.Msg.GetRoles() { + var metadata map[string]any + if md := ro.GetMetadata(); md != nil { + metadata = md.AsMap() + } current = append(current, currentRole{ ID: ro.GetId(), Name: ro.GetName(), @@ -140,6 +143,7 @@ func (r *RoleReconciler) fetchCurrent(ctx context.Context) ([]currentRole, error Description: metadataString(ro.GetMetadata(), descriptionKey), Permissions: ro.GetPermissions(), Scopes: ro.GetScopes(), + Metadata: metadata, }) } return current, nil @@ -159,14 +163,14 @@ func metadataString(md *structpb.Struct, key string) string { func (r *RoleReconciler) apply(ctx context.Context, op roleOp) error { switch op.action { case opAdd: - body, err := roleBody(op.spec) + body, err := roleBody(op.spec, op.baseMetadata) if err != nil { return err } _, err = r.client.CreateRole(ctx, authReq(&frontierv1beta1.CreateRoleRequest{Body: body}, r.header)) return err case opUpdate: - body, err := roleBody(op.spec) + body, err := roleBody(op.spec, op.baseMetadata) if err != nil { return err } @@ -180,10 +184,20 @@ func (r *RoleReconciler) apply(ctx context.Context, op roleOp) error { } } -func roleBody(spec RoleSpec) (*frontierv1beta1.RoleRequestBody, error) { - fields := map[string]any{managedByKey: managedByValue} +// roleBody builds the request body for an add or update. base is the role's +// current metadata (nil for a new role); the reconciler-managed keys are merged +// over a copy of it, so other metadata keys an operator set are kept, not +// dropped. An empty managed description clears the key, matching a reset. +func roleBody(spec RoleSpec, base map[string]any) (*frontierv1beta1.RoleRequestBody, error) { + fields := map[string]any{} + for k, v := range base { + fields[k] = v + } + fields[managedByKey] = managedByValue if spec.Description != "" { fields[descriptionKey] = spec.Description + } else { + delete(fields, descriptionKey) } md, err := structpb.NewStruct(fields) if err != nil { diff --git a/internal/reconcile/role_reconciler_test.go b/internal/reconcile/role_reconciler_test.go index b7cf87254..82578bbee 100644 --- a/internal/reconcile/role_reconciler_test.go +++ b/internal/reconcile/role_reconciler_test.go @@ -97,6 +97,36 @@ func TestRoleReconciler(t *testing.T) { assert.Equal(t, []string{"r2"}, api.deleted) }) + t.Run("an update keeps metadata keys the reconciler does not manage", func(t *testing.T) { + md, _ := structpb.NewStruct(map[string]any{"team": "compute", descriptionKey: "old", managedByKey: managedByValue}) + role := rolePB("r1", "compute_manager", "Compute Manager", []string{"compute_order_get"}) + role.Metadata = md + api := &fakeRoleAPI{roles: []*frontierv1beta1.Role{ownerDefaultPB(), role}} + // only permissions change; team must survive the update + spec := []byte("- {name: compute_manager, permissions: [compute_order_get, compute_order_update]}\n") + + rep, err := NewRoleReconciler(api, "").Reconcile(context.Background(), spec, false) + + assert.NoError(t, err) + assert.Equal(t, 1, rep.Applied) + if body := api.updated["r1"]; assert.NotNil(t, body) { + f := body.GetMetadata().GetFields() + assert.Equal(t, "compute", f["team"].GetStringValue()) // preserved, not dropped + assert.Equal(t, managedByValue, f[managedByKey].GetStringValue()) // still stamped + assert.Equal(t, "old", f[descriptionKey].GetStringValue()) // kept + } + }) + + t.Run("an unknown field in the spec fails the plan", func(t *testing.T) { + api := &fakeRoleAPI{} + spec := []byte("- {name: new_role, permissions: [compute_order_get], delet: true}\n") + + _, err := NewRoleReconciler(api, "").Reconcile(context.Background(), spec, true) + + assert.ErrorContains(t, err, "parse Role spec") + assert.Empty(t, api.created) + }) + t.Run("updates a predefined role's title and permissions by id", func(t *testing.T) { api := &fakeRoleAPI{roles: []*frontierv1beta1.Role{ownerDefaultPB()}} spec := []byte(` From d2cabb94e3a8fdb3fe8d399356d7a5e6a5565d54 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Thu, 16 Jul 2026 14:19:57 +0530 Subject: [PATCH 3/3] docs(reconcile): note predefined-role reset edges and that only custom roles can be deleted --- cmd/reconcile.go | 6 +++--- docs/content/docs/reconcile.mdx | 5 +++++ docs/content/docs/reference/cli.mdx | 5 +++-- 3 files changed, 11 insertions(+), 5 deletions(-) diff --git a/cmd/reconcile.go b/cmd/reconcile.go index 4b2ecff81..b711a3a08 100644 --- a/cmd/reconcile.go +++ b/cmd/reconcile.go @@ -24,9 +24,9 @@ func ReconcileCommand(cliConfig *Config) *cli.Command { Kinds: PlatformUser (platform admins and members), Permission (custom permissions), and Role (platform-level roles). Deleting a permission or a - role needs an explicit 'delete: true' on its entry; nothing is deleted by - omission. Log in as a superuser (for example the bootstrap service account) - with --header. + custom role needs an explicit 'delete: true' on its entry; nothing is deleted + by omission, and a predefined role cannot be deleted. Log in as a superuser + (for example the bootstrap service account) with --header. Use "frontier export " to print the current state in this file format. `), diff --git a/docs/content/docs/reconcile.mdx b/docs/content/docs/reconcile.mdx index 4dccc2cd4..4ac74dd5f 100644 --- a/docs/content/docs/reconcile.mdx +++ b/docs/content/docs/reconcile.mdx @@ -138,6 +138,11 @@ role you do not list at all resets fully. The plan marks these resets, so a hand change to a predefined role shows up as an update back to the default. You cannot delete a predefined role; the server recreates it at boot. +Two edge cases to know. A field that is empty on the server — say the scopes on a role +created before that field existed — is changed only when you list it, so exports keep +round-tripping cleanly. And an empty permission or scope list is rejected: you cannot blank +those out through the file. + Because a predefined role resets to its default, removing its entry from the file is a real change. If an entry adds permissions to `app_project_viewer`, deleting that entry takes those permissions away on the next apply. diff --git a/docs/content/docs/reference/cli.mdx b/docs/content/docs/reference/cli.mdx index f8e458817..47a2b39a4 100644 --- a/docs/content/docs/reference/cli.mdx +++ b/docs/content/docs/reference/cli.mdx @@ -248,8 +248,9 @@ View a project Make platform resources match a desired-state YAML file, through the admin API. Supported kinds: `PlatformUser` (anyone listed is added, anyone not listed is removed), `Permission` (custom permissions), and `Role` -(platform-level roles). Deleting a permission or a role needs an explicit -`delete: true` on its entry; nothing is deleted by omission. Use +(platform-level roles). Deleting a permission or a custom role needs an +explicit `delete: true` on its entry; nothing is deleted by omission, and a +predefined role cannot be deleted. Use `frontier export` to print the current state in this file format, and see the [Reconcile guide](../reconcile.md) for the full flow.