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/docs/content/docs/reference/cli.mdx b/docs/content/docs/reference/cli.mdx index 779cf9d87..43ec482c1 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`. +changes nothing. Supported kinds: `PlatformUser`, `Permission`, `Role`. ``` $ frontier export platformuser -H "Authorization:Basic " > platform-users.yaml @@ -245,9 +245,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. +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. ``` $ frontier reconcile -f platform-users.yaml --dry-run -H "Authorization:Basic " 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..d483546f2 --- /dev/null +++ b/internal/reconcile/permission.go @@ -0,0 +1,121 @@ +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 + } + + seenDelete := map[string]bool{} + 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 := seenDelete[slug]; dup { + if prev != s.Delete { + return nil, fmt.Errorf("permission %s is listed both with and without delete", s) + } + continue + } + seenDelete[slug] = s.Delete + 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..b45de60d7 --- /dev/null +++ b/internal/reconcile/permission_test.go @@ -0,0 +1,78 @@ +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("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/role.go b/internal/reconcile/role.go new file mode 100644 index 000000000..81d8e2b41 --- /dev/null +++ b/internal/reconcile/role.go @@ -0,0 +1,205 @@ +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. A field that is present is managed; a field that is omitted keeps +// its server value. Custom roles must list permissions. Predefined roles may +// override title, permissions, or scopes, and cannot be deleted — bootstrap +// recreates a missing predefined role on the next boot. +type RoleSpec struct { + Name string `yaml:"name"` + Title string `yaml:"title,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. +type currentRole struct { + ID string + Name string + Title 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 +} + +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 && s.Permissions == nil { + return fmt.Errorf("a custom role must list its permissions (use [] for none)") + } + return nil +} + +// diffRoles returns the ops that make the current platform roles match the +// desired spec. Every custom role on the server must appear in the spec — +// kept, or marked delete — so nothing is ever removed by omission. Predefined +// roles are managed only when listed; unlisted ones are left alone. +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{}{} + accounted := map[string]struct{}{} + var adds, updates, removes []roleOp + 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{}{} + accounted[s.Name] = struct{}{} + + cur, exists := byName[s.Name] + if s.Delete { + if exists { + removes = append(removes, roleOp{action: opRemove, spec: s, id: cur.ID}) + } + continue + } + + desiredPerms := cur.Permissions + if s.Permissions != nil { + desiredPerms = normalizePermissions(s.Permissions) + } + + if !exists { + if isPredefined { + return nil, fmt.Errorf("predefined role %q not found on the server; bootstrap creates it at boot", s.Name) + } + adds = append(adds, roleOp{action: opAdd, spec: RoleSpec{ + Name: s.Name, + Title: s.Title, + Permissions: desiredPerms, + Scopes: sortedCopy(s.Scopes), + }}) + continue + } + + var changes []string + merged := RoleSpec{Name: s.Name, Title: cur.Title, 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.Permissions != nil && !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 := accounted[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 +} diff --git a/internal/reconcile/role_reconciler.go b/internal/reconcile/role_reconciler.go new file mode 100644 index 000000000..d856b3941 --- /dev/null +++ b/internal/reconcile/role_reconciler.go @@ -0,0 +1,168 @@ +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" +) + +// 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, and only with the differing fields. +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, + Permissions: sortedCopy(c.Permissions), + Scopes: sortedCopy(c.Scopes), + }) + continue + } + var entry RoleSpec + if c.Title != def.Title { + entry.Title = c.Title + } + if !stringSetsEqual(sortedCopy(c.Permissions), normalizePermissions(def.Permissions)) { + entry.Permissions = sortedCopy(c.Permissions) + } + if entry.Title == "" && entry.Permissions == nil { + continue + } + entry.Name = c.Name + 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(), + Permissions: ro.GetPermissions(), + Scopes: ro.GetScopes(), + }) + } + return current, nil +} + +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) { + md, err := structpb.NewStruct(map[string]any{managedByKey: managedByValue}) + 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..7b977642b --- /dev/null +++ b/internal/reconcile/role_reconciler_test.go @@ -0,0 +1,131 @@ +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" +) + +type fakeRoleAPI struct { + roles []*frontierv1beta1.Role + created []*frontierv1beta1.RoleRequestBody + updated map[string]*frontierv1beta1.RoleRequestBody + deleted []string +} + +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) { + 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.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 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()) // kept from server + } + }) + + 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("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..59f3045b7 --- /dev/null +++ b/internal/reconcile/role_test.go @@ -0,0 +1,126 @@ +package reconcile + +import ( + "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 skipped", func(t *testing.T) { + ops, err := diffRoles(keepCustom, current) + assert.NoError(t, err) + assert.Empty(t, ops) + }) + + 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("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 permissions", func(t *testing.T) { + _, err := diffRoles([]RoleSpec{{Name: "compute_manager", Title: "X"}}, current) + assert.ErrorContains(t, err, "must list its permissions") + }) + + 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) + }) +}