diff --git a/internal/api/v1beta1connect/permission.go b/internal/api/v1beta1connect/permission.go index 973c30834..601681c5b 100644 --- a/internal/api/v1beta1connect/permission.go +++ b/internal/api/v1beta1connect/permission.go @@ -26,11 +26,12 @@ func (h *ConnectHandler) CreatePermission(ctx context.Context, request *connect. if permName == "" || permNamespace == "" { return nil, connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation) } - if !schema.IsValidPermissionName(permName) { - return nil, connect.NewError(connect.CodeInvalidArgument, errors.New("permission name cannot contain special characters")) - } - if !schema.IsValidPermissionNamespace(permNamespace) { - return nil, connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation) + // One shared check for the verb grammar, the namespace grammar, the reserved + // verbs, and the slug length, so the create API and the reconcile plan agree + // and a permission that would fail when SpiceDB compiles the schema is + // rejected up front with a message that says which rule it broke. + if err := schema.ValidateCustomPermission(permNamespace, permName); err != nil { + return nil, connect.NewError(connect.CodeInvalidArgument, err) } permissionSlugs = append(permissionSlugs, schema.FQPermissionNameFromNamespace(permNamespace, permName)) @@ -123,11 +124,10 @@ func (h *ConnectHandler) UpdatePermission(ctx context.Context, request *connect. if permNamespace == "" || permName == "" { return nil, connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation) } - if !schema.IsValidPermissionName(permName) { - return nil, connect.NewError(connect.CodeInvalidArgument, errors.New("permission name cannot contain special characters")) - } - if !schema.IsValidPermissionNamespace(permNamespace) { - return nil, connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation) + // Same shared check as create, so an update cannot rename a permission to a key + // SpiceDB will reject and then break the next schema compile at boot. + if err := schema.ValidateCustomPermission(permNamespace, permName); err != nil { + return nil, connect.NewError(connect.CodeInvalidArgument, err) } updatedPermission, err := h.permissionService.Update(ctx, permission.Permission{ ID: request.Msg.GetId(), diff --git a/internal/api/v1beta1connect/permission_test.go b/internal/api/v1beta1connect/permission_test.go index 0f1096386..7fa248eb9 100644 --- a/internal/api/v1beta1connect/permission_test.go +++ b/internal/api/v1beta1connect/permission_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "strings" "testing" "time" @@ -24,7 +25,7 @@ var ( testPermissions = []permission.Permission{ { ID: uuid.New().String(), - Name: "Read", + Name: "read", NamespaceID: "app/resource", Metadata: map[string]any{}, CreatedAt: time.Time{}, @@ -32,14 +33,14 @@ var ( }, { ID: uuid.New().String(), - Name: "Write", + Name: "write", NamespaceID: "app/resource", CreatedAt: time.Time{}, UpdatedAt: time.Time{}, }, { ID: uuid.New().String(), - Name: "Manage", + Name: "manage", NamespaceID: "app/resource", CreatedAt: time.Time{}, UpdatedAt: time.Time{}, @@ -198,6 +199,105 @@ func TestHandler_CreatePermission(t *testing.T) { }), wantErr: nil, }, + { + name: "should return success if permission service return nil error with permission key", + setup: func(as *mocks.PermissionService, bs *mocks.BootstrapService) { + bs.EXPECT().AppendSchema(mock.AnythingOfType("context.backgroundCtx"), schema.ServiceDefinition{ + Permissions: []schema.ResourcePermission{ + { + Name: testPermissions[testPermissionIdx].Name + "0", + Namespace: testPermissions[testPermissionIdx].NamespaceID, + }, + }, + }).Return(nil) + as.EXPECT().List(mock.Anything, permission.Filter{ + Slugs: []string{ + schema.FQPermissionNameFromNamespace(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name+"0"), + }, + }).Return([]permission.Permission{ + { + ID: testPermissions[testPermissionIdx].ID, + Name: testPermissions[testPermissionIdx].Name + "0", + NamespaceID: testPermissions[testPermissionIdx].NamespaceID, + }, + }, nil) + }, + request: connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ + Bodies: []*frontierv1beta1.PermissionRequestBody{ + { + Key: schema.PermissionKeyFromNamespaceAndName(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name+"0"), + }, + }, + }), + want: connect.NewResponse(&frontierv1beta1.CreatePermissionResponse{ + Permissions: []*frontierv1beta1.Permission{ + { + Id: testPermissions[testPermissionIdx].ID, + Name: testPermissions[testPermissionIdx].Name + "0", + Namespace: testPermissions[testPermissionIdx].NamespaceID, + Key: schema.PermissionKeyFromNamespaceAndName(testPermissions[testPermissionIdx].NamespaceID, testPermissions[testPermissionIdx].Name+"0"), + CreatedAt: timestamppb.New(testPermissions[testPermissionIdx].CreatedAt), + UpdatedAt: timestamppb.New(testPermissions[testPermissionIdx].UpdatedAt), + }, + }, + }), + wantErr: nil, + }, + { + name: "should reject an uppercase verb with the grammar error", + setup: func(as *mocks.PermissionService, bs *mocks.BootstrapService) {}, + request: connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ + Bodies: []*frontierv1beta1.PermissionRequestBody{ + {Key: "compute.order.Read"}, + }, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, schema.ValidateCustomPermission("compute/order", "Read")), + }, + { + name: "should reject a too-short verb with the grammar error", + setup: func(as *mocks.PermissionService, bs *mocks.BootstrapService) {}, + request: connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ + Bodies: []*frontierv1beta1.PermissionRequestBody{ + {Key: "compute.order.id"}, + }, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, schema.ValidateCustomPermission("compute/order", "id")), + }, + { + name: "should reject a verb that collides with a generated relation", + setup: func(as *mocks.PermissionService, bs *mocks.BootstrapService) {}, + request: connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ + Bodies: []*frontierv1beta1.PermissionRequestBody{ + {Key: "compute.order.owner"}, + }, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, schema.ValidateCustomPermission("compute/order", "owner")), + }, + { + name: "should reject a namespace with an underscore in a part", + setup: func(as *mocks.PermissionService, bs *mocks.BootstrapService) {}, + request: connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ + Bodies: []*frontierv1beta1.PermissionRequestBody{ + {Key: "res_order.item.get"}, + }, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, schema.ValidateCustomPermission("res_order/item", "get")), + }, + { + name: "should reject a permission whose flattened slug is too long", + setup: func(as *mocks.PermissionService, bs *mocks.BootstrapService) {}, + request: connect.NewRequest(&frontierv1beta1.CreatePermissionRequest{ + Bodies: []*frontierv1beta1.PermissionRequestBody{ + {Key: strings.Repeat("a", 30) + "." + strings.Repeat("b", 30) + ".get"}, + }, + }), + want: nil, + wantErr: connect.NewError(connect.CodeInvalidArgument, schema.ValidateCustomPermission(strings.Repeat("a", 30)+"/"+strings.Repeat("b", 30), "get")), + }, } for _, tt := range tests { @@ -317,7 +417,7 @@ func TestHandler_UpdatePermission(t *testing.T) { wantErr: connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation), }, { - name: "should return bad request error if key name has special characters", + name: "should return bad request error if the key verb breaks the grammar", setup: func(as *mocks.PermissionService) {}, request: connect.NewRequest(&frontierv1beta1.UpdatePermissionRequest{ Id: testPermissions[testPermissionIdx].ID, @@ -326,7 +426,7 @@ func TestHandler_UpdatePermission(t *testing.T) { }, }), want: nil, - wantErr: connect.NewError(connect.CodeInvalidArgument, errors.New("permission name cannot contain special characters")), + wantErr: connect.NewError(connect.CodeInvalidArgument, schema.ValidateCustomPermission("app/resource", "we$rd")), }, { name: "should return bad request error if a key part is empty", @@ -350,7 +450,7 @@ func TestHandler_UpdatePermission(t *testing.T) { }, }), want: nil, - wantErr: connect.NewError(connect.CodeInvalidArgument, ErrPermissionKeyNotation), + wantErr: connect.NewError(connect.CodeInvalidArgument, schema.ValidateCustomPermission("Ab/resource", "get")), }, { name: "should return bad request error if deprecated fields are sent without key", diff --git a/internal/bootstrap/schema/schema.go b/internal/bootstrap/schema/schema.go index 612f76be9..3ab54a2a4 100644 --- a/internal/bootstrap/schema/schema.go +++ b/internal/bootstrap/schema/schema.go @@ -299,27 +299,29 @@ func IsSystemNamespace(namespace string) bool { namespace == PATPrincipal || namespace == PlatformNamespace } -// IsValidPermissionName checks if the provided name is a valid permission name +// permissionNameRe matches a permission verb. The verb becomes a SpiceDB +// relation name directly, so it must satisfy SpiceDB's relation grammar +// (^[a-z][a-z0-9_]{1,62}[a-z0-9]$): start with a lowercase letter, then +// lowercase alphanumerics, three to sixty-four characters. Underscore is left +// out here because the slug joins service, resource, and verb with "_", so an +// underscore in the verb is treated the same as in a namespace part. +var permissionNameRe = regexp.MustCompile(`^[a-z][a-z0-9]{2,63}$`) + +// IsValidPermissionName reports whether name is a valid permission verb that +// SpiceDB will accept as a relation name. func IsValidPermissionName(name string) bool { - if name == "" { - return false - } - // check if name contains anything other than alphanumeric characters - for _, r := range name { - if !((r >= 'a' && r <= 'z') || (r >= 'A' && r <= 'Z') || (r >= '0' && r <= '9')) { - return false - } - } - return true + return permissionNameRe.MatchString(name) } -// 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]+$`) +// permissionNamespaceRe matches a custom permission namespace: service/resource, +// where each part is a valid SpiceDB object type segment (^[a-z][a-z0-9_]{1,62} +// [a-z0-9]$): start with a lowercase letter, then lowercase alphanumerics, three +// to sixty-four 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 and a leading digit are forbidden because SpiceDB rejects +// them in an object type name, so such a namespace could never be stored anyway. +var permissionNamespaceRe = regexp.MustCompile(`^[a-z][a-z0-9]{2,63}/[a-z][a-z0-9]{2,63}$`) // IsValidPermissionNamespace reports whether namespace is in service/resource // form with each part lowercase alphanumeric. @@ -327,6 +329,59 @@ func IsValidPermissionNamespace(namespace string) bool { return permissionNamespaceRe.MatchString(namespace) } +// maxPermissionSlugLen is SpiceDB's ceiling on a relation name. The reconcile and +// API paths flatten a permission to the slug service_resource_verb and register +// that slug as a SpiceDB relation, so a longer slug is rejected when the schema is +// compiled. +const maxPermissionSlugLen = 64 + +// PermissionSlugWithinLimit reports whether the flattened slug +// service_resource_verb fits SpiceDB's relation-name limit. Each part can be up to +// sixty-four characters on its own, so a valid namespace and verb can still combine +// into a slug that SpiceDB will not store. +func PermissionSlugWithinLimit(namespace, name string) bool { + return len(FQPermissionNameFromNamespace(namespace, name)) <= maxPermissionSlugLen +} + +// reservedPermissionVerbs are the relation names the schema generator adds to +// every custom resource definition (see internal/bootstrap/generator.go's owner, +// project, and granted relations). A permission whose verb equals one of these +// makes the generator declare the same relation twice, which SpiceDB rejects when +// it compiles the schema. +var reservedPermissionVerbs = map[string]bool{ + OwnerRelationName: true, // owner + ProjectRelationName: true, // project + RoleGrantRelationName: true, // granted +} + +// IsReservedPermissionVerb reports whether name collides with a relation the +// generator always adds to a custom resource. +func IsReservedPermissionVerb(name string) bool { + return reservedPermissionVerbs[strings.ToLower(name)] +} + +// ValidateCustomPermission checks that a custom permission's namespace and verb +// can be created and then reconciled without a plan that passes and an apply that +// fails: the verb and each namespace part must satisfy SpiceDB's grammar, the verb +// must not collide with a generated relation, and the flattened slug must fit +// SpiceDB's relation-name limit. Both the reconcile validator and the +// CreatePermission handler call this, so the two paths agree. +func ValidateCustomPermission(namespace, name string) error { + if !IsValidPermissionName(name) { + return fmt.Errorf("invalid permission verb %q: must be lowercase, start with a letter, and be at least three characters", name) + } + if IsReservedPermissionVerb(name) { + return fmt.Errorf("permission verb %q is reserved: the schema already defines a relation with that name on every resource", name) + } + if !IsValidPermissionNamespace(namespace) { + return fmt.Errorf("invalid permission namespace %q: service and resource must each be lowercase alphanumeric and three to sixty-four characters", namespace) + } + if !PermissionSlugWithinLimit(namespace, name) { + return fmt.Errorf("permission %s:%s is too long: the flattened service_resource_verb must be at most 64 characters", namespace, name) + } + return nil +} + 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 aa03d6af9..5af7e0ba8 100644 --- a/internal/bootstrap/schema/schema_test.go +++ b/internal/bootstrap/schema/schema_test.go @@ -1,6 +1,7 @@ package schema_test import ( + "strings" "testing" "github.com/raystack/frontier/internal/bootstrap/schema" @@ -106,7 +107,11 @@ func TestIsValidPermissionNamespace(t *testing.T) { {"user/project", true}, {"org/user", true}, {"compute/disk", true}, - {"a1/b2", true}, + {"a1/b2", false}, // parts shorter than 3 are not valid SpiceDB object types + {"ab/order", false}, // service part shorter than 3 + {"compute/ab", false}, // resource part shorter than 3 + {"1compute/order", false}, // a part cannot start with a digit + {"compute/2order", false}, // a part cannot start with a digit {"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 @@ -127,6 +132,115 @@ func TestIsValidPermissionNamespace(t *testing.T) { } } +func TestIsValidPermissionName(t *testing.T) { + // A permission's verb becomes a SpiceDB relation name directly, so it must + // satisfy SpiceDB's relation grammar: start with a lowercase letter, then + // lowercase alphanumerics, at least three characters. Underscore is left out + // because the slug joins service, resource, and verb with "_". + tests := []struct { + name string + want bool + }{ + {"get", true}, + {"create", true}, + {"list", true}, + {"abc", true}, // exactly three characters + {"id", false}, // shorter than three characters + {"ab", false}, + {"Get", false}, // uppercase is not a SpiceDB relation + {"getAll", false}, // uppercase anywhere + {"1get", false}, // cannot start with a digit + {"get_all", false}, // underscore is not allowed in a verb + {"get-all", false}, // hyphen is not alphanumeric + {"", false}, // empty + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := schema.IsValidPermissionName(tt.name); got != tt.want { + t.Errorf("IsValidPermissionName(%q) = %v, want %v", tt.name, got, tt.want) + } + }) + } +} + +func TestPermissionSlugWithinLimit(t *testing.T) { + // The slug service_resource_verb becomes a SpiceDB relation, which is capped at + // sixty-four characters. Each part is valid on its own, but together they can + // overflow, so a plan can pass and the schema still fail to compile. + tests := []struct { + ns, name string + want bool + }{ + {"compute/order", "get", true}, + // 30 + 29 + 3 plus the two underscores is exactly sixty-four + {strings.Repeat("a", 30) + "/" + strings.Repeat("b", 29), "get", true}, + // one more character tips it over the limit + {strings.Repeat("a", 30) + "/" + strings.Repeat("b", 30), "get", false}, + } + for _, tt := range tests { + t.Run(tt.ns+"."+tt.name, func(t *testing.T) { + if got := schema.PermissionSlugWithinLimit(tt.ns, tt.name); got != tt.want { + t.Errorf("PermissionSlugWithinLimit(%q, %q) = %v, want %v", tt.ns, tt.name, got, tt.want) + } + }) + } +} + +func TestIsReservedPermissionVerb(t *testing.T) { + // These are the relations the generator adds to every custom resource, so a + // verb equal to one of them would declare the same relation twice. + tests := []struct { + name string + want bool + }{ + {"owner", true}, + {"project", true}, + {"granted", true}, + {"Owner", true}, // reserved check is case-insensitive + {"get", false}, + {"delete", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := schema.IsReservedPermissionVerb(tt.name); got != tt.want { + t.Errorf("IsReservedPermissionVerb(%q) = %v, want %v", tt.name, got, tt.want) + } + }) + } +} + +func TestValidateCustomPermission(t *testing.T) { + tests := []struct { + title string + ns string + name string + wantErr string // substring the error must contain; empty means no error + }{ + {"valid", "compute/order", "get", ""}, + {"uppercase verb", "compute/order", "Read", "invalid permission verb"}, + {"short verb", "compute/order", "id", "invalid permission verb"}, + {"reserved verb owner", "compute/order", "owner", "reserved"}, + {"reserved verb granted", "compute/order", "granted", "reserved"}, + {"underscore namespace", "res_order/item", "get", "invalid permission namespace"}, + {"missing resource", "compute", "get", "invalid permission namespace"}, + {"slug too long", strings.Repeat("a", 30) + "/" + strings.Repeat("b", 30), "get", "too long"}, + } + for _, tt := range tests { + t.Run(tt.title, func(t *testing.T) { + err := schema.ValidateCustomPermission(tt.ns, tt.name) + if tt.wantErr == "" { + if err != nil { + t.Errorf("ValidateCustomPermission(%q, %q) = %v, want nil", tt.ns, tt.name, err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("ValidateCustomPermission(%q, %q) = %v, want error containing %q", tt.ns, tt.name, err, tt.wantErr) + } + }) + } +} + func TestIsBootstrapServiceUser(t *testing.T) { tests := []struct { name string diff --git a/internal/reconcile/permission.go b/internal/reconcile/permission.go index bce219234..1c4543961 100644 --- a/internal/reconcile/permission.go +++ b/internal/reconcile/permission.go @@ -39,23 +39,14 @@ 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) - } - // 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 + // One shared check so the reconcile plan and the CreatePermission API agree on + // what a valid custom permission is: SpiceDB grammar, no reserved verb, and a + // slug that fits SpiceDB's relation-name limit. This stops a plan that passes and + // then fails when the schema compiles. + return schema.ValidateCustomPermission(s.Namespace, s.Name) } // currentPermission is one custom permission as returned by ListPermissions. diff --git a/internal/reconcile/permission_test.go b/internal/reconcile/permission_test.go index 3c442e35c..a8d358e96 100644 --- a/internal/reconcile/permission_test.go +++ b/internal/reconcile/permission_test.go @@ -1,6 +1,7 @@ package reconcile import ( + "strings" "testing" "github.com/stretchr/testify/assert" @@ -93,6 +94,29 @@ func TestDiffPermissions(t *testing.T) { } }) + t.Run("rejects a permission whose flattened slug is too long", func(t *testing.T) { + // Each part is valid on its own, but the slug service_resource_verb overflows + // SpiceDB's sixty-four character relation limit, so it must fail the plan + // rather than pass and then fail when the schema compiles. + ns := strings.Repeat("a", 30) + "/" + strings.Repeat("b", 30) + _, err := diffPermissions([]PermissionSpec{{Namespace: ns, Name: "get"}}, nil) + if assert.Error(t, err) { + assert.ErrorContains(t, err, "too long") + } + }) + + t.Run("rejects a verb that collides with a generated relation", func(t *testing.T) { + // The generator adds owner, project, and granted to every custom resource, so + // a verb equal to one of them would declare the same relation twice and the + // schema would fail to compile. The plan must reject it up front. + for _, name := range []string{"owner", "project", "granted"} { + _, err := diffPermissions([]PermissionSpec{{Namespace: "compute/order", Name: name}}, nil) + if assert.Error(t, err, name) { + assert.ErrorContains(t, err, "reserved") + } + } + }) + t.Run("rejects base-schema namespaces and bad shapes", func(t *testing.T) { for _, s := range []PermissionSpec{ {Namespace: "app/organization", Name: "hack"},