diff --git a/internal/bootstrap/schema/schema.go b/internal/bootstrap/schema/schema.go index 60b52fd0b..475065085 100644 --- a/internal/bootstrap/schema/schema.go +++ b/internal/bootstrap/schema/schema.go @@ -4,6 +4,7 @@ import ( _ "embed" "errors" "fmt" + "regexp" "strings" "github.com/google/uuid" @@ -311,6 +312,20 @@ func IsValidPermissionName(name string) bool { return true } +// permissionNamespaceRe matches a custom permission namespace: service/resource +// with each part one or more lowercase alphanumeric characters. It forbids the +// underscore inside a part, because FQPermissionNameFromNamespace joins service, +// resource, and the verb with "_", so an underscore in a part would let two +// different namespaces flatten to the same slug. Uppercase is forbidden because +// SpiceDB object type names are lowercase, so it could never be stored anyway. +var permissionNamespaceRe = regexp.MustCompile(`^[a-z0-9]+/[a-z0-9]+$`) + +// IsValidPermissionNamespace reports whether namespace is in service/resource +// form with each part lowercase alphanumeric. +func IsValidPermissionNamespace(namespace string) bool { + return permissionNamespaceRe.MatchString(namespace) +} + func IsPlatformPermission(name string) bool { name = strings.ToLower(name) return name == PlatformSudoPermission || name == PlatformCheckPermission diff --git a/internal/bootstrap/schema/schema_test.go b/internal/bootstrap/schema/schema_test.go index deeb980d9..6b75057cd 100644 --- a/internal/bootstrap/schema/schema_test.go +++ b/internal/bootstrap/schema/schema_test.go @@ -50,6 +50,36 @@ func TestFQPermissionNameFromNamespace(t *testing.T) { } } +func TestIsValidPermissionNamespace(t *testing.T) { + tests := []struct { + ns string + want bool + }{ + {"resource/aoi", true}, + {"user/project", true}, + {"org/user", true}, + {"compute/disk", true}, + {"a1/b2", true}, + {"resource_order/item", false}, // underscore in a part collides on the slug + {"resource/order_item", false}, + {"Compute/order", false}, // uppercase is not a SpiceDB object type + {"compute/Order", false}, + {"compute", false}, // one part + {"a/b/c", false}, // three parts + {"/x", false}, // empty part + {"x/", false}, // empty part + {"", false}, // empty + {"comp-ute/x", false}, // hyphen is not alphanumeric + } + for _, tt := range tests { + t.Run(tt.ns, func(t *testing.T) { + if got := schema.IsValidPermissionNamespace(tt.ns); got != tt.want { + t.Errorf("IsValidPermissionNamespace(%q) = %v, want %v", tt.ns, got, tt.want) + } + }) + } +} + func TestIsBootstrapServiceUser(t *testing.T) { tests := []struct { name string diff --git a/internal/reconcile/permission.go b/internal/reconcile/permission.go index 33616efa9..bce219234 100644 --- a/internal/reconcile/permission.go +++ b/internal/reconcile/permission.go @@ -48,6 +48,13 @@ func validatePermissionSpec(s PermissionSpec) error { 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) } + // The slug joins service, resource, and verb with "_", so an underscore inside + // a namespace part would make two different namespaces flatten to the same slug + // and be silently treated as one. Require each part to be lowercase alphanumeric + // so the slug is one-to-one and a plan cannot mistake one namespace for another. + if !schema.IsValidPermissionNamespace(s.Namespace) { + return fmt.Errorf("invalid namespace %q (each of service/resource must be lowercase alphanumeric)", s.Namespace) + } return nil } diff --git a/internal/reconcile/permission_reconciler_test.go b/internal/reconcile/permission_reconciler_test.go index 1724b5d01..7e53c8928 100644 --- a/internal/reconcile/permission_reconciler_test.go +++ b/internal/reconcile/permission_reconciler_test.go @@ -77,6 +77,18 @@ func TestPermissionReconciler(t *testing.T) { assert.Empty(t, api.created) }) + t.Run("an ambiguous namespace fails Validate before any server call", func(t *testing.T) { + // The charset check is server-free, so a namespace that would collide on the + // slug must fail the whole file up front (rule 3), not at apply. + api := &fakePermissionAPI{} + spec := []byte("- {namespace: resource_order/item, name: get}\n") + + err := NewPermissionReconciler(api, "").Validate(spec) + + assert.ErrorContains(t, err, "resource_order/item") + 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/permission_test.go b/internal/reconcile/permission_test.go index ade1237e6..3c442e35c 100644 --- a/internal/reconcile/permission_test.go +++ b/internal/reconcile/permission_test.go @@ -63,14 +63,34 @@ func TestDiffPermissions(t *testing.T) { 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("a file namespace that would collide with a server slug is rejected, not absorbed", func(t *testing.T) { + // The server stores resource/order_item; the file lists a genuinely different + // namespace resource_order/item that flattens to the same slug. The diff used + // to treat it as already present and plan zero ops (the rule 2 gap). The + // ambiguous namespace is now rejected at validation, so it cannot be absorbed. + server := []currentPermission{{ID: "p1", Namespace: "resource/order_item", Name: "get"}} + _, err := diffPermissions([]PermissionSpec{{Namespace: "resource_order/item", Name: "get"}}, server) + assert.ErrorContains(t, err, "resource_order/item") + }) + + t.Run("rejects a namespace with an underscore or uppercase in a part", func(t *testing.T) { + // The slug joins service, resource, and verb with "_", so an underscore in a + // part makes two namespaces flatten to one slug; uppercase cannot be a + // SpiceDB object type. Both are rejected so the slug stays one-to-one. + for _, ns := range []string{"resource_order/item", "resource/order_item", "Compute/order", "compute/Order"} { + _, err := diffPermissions([]PermissionSpec{{Namespace: ns, Name: "get"}}, nil) + if assert.Error(t, err, ns) { + assert.ErrorContains(t, err, "namespace") + } + } + }) + + t.Run("accepts valid custom namespaces", func(t *testing.T) { + for _, ns := range []string{"resource/aoi", "user/project", "org/user", "compute/disk"} { + ops, err := diffPermissions([]PermissionSpec{{Namespace: ns, Name: "get"}}, nil) + assert.NoError(t, err, ns) + assert.Len(t, ops, 1) // a valid new permission plans an add + } }) t.Run("rejects base-schema namespaces and bad shapes", func(t *testing.T) {