From 7ec98774ac0c522d90fec1abcaca295ce8f1fb4f Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Tue, 11 Aug 2026 14:29:30 +0530 Subject: [PATCH 1/3] fix(bootstrap): return the error when listing existing permissions in AppendSchema --- internal/bootstrap/service.go | 2 +- internal/bootstrap/service_test.go | 35 ++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/internal/bootstrap/service.go b/internal/bootstrap/service.go index a4d9dd26b0..3ed9119288 100644 --- a/internal/bootstrap/service.go +++ b/internal/bootstrap/service.go @@ -185,7 +185,7 @@ func (s Service) AppendSchema(ctx context.Context, customServiceDefinition schem existingPermissions, err := s.permissionService.List(ctx, permission.Filter{}) if err != nil { - return nil + return fmt.Errorf("AppendSchema: listing existing permissions: %w", err) } for _, existingPermission := range existingPermissions { description := "" diff --git a/internal/bootstrap/service_test.go b/internal/bootstrap/service_test.go index 28c1f78c58..c4ebf88007 100644 --- a/internal/bootstrap/service_test.go +++ b/internal/bootstrap/service_test.go @@ -5,6 +5,7 @@ import ( "errors" "testing" + "github.com/raystack/frontier/core/permission" "github.com/raystack/frontier/core/relation" "github.com/raystack/frontier/core/role" "github.com/raystack/frontier/internal/bootstrap/schema" @@ -55,6 +56,24 @@ func (m *mockRelationService) Delete(ctx context.Context, rel relation.Relation) return args.Error(0) } +// mockPermissionService implements bootstrap.PermissionService +type mockPermissionService struct { + mock.Mock +} + +func (m *mockPermissionService) List(ctx context.Context, flt permission.Filter) ([]permission.Permission, error) { + args := m.Called(ctx, flt) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]permission.Permission), args.Error(1) +} + +func (m *mockPermissionService) Upsert(ctx context.Context, action permission.Permission) (permission.Permission, error) { + args := m.Called(ctx, action) + return args.Get(0).(permission.Permission), args.Error(1) +} + func Test_migratePATRelations(t *testing.T) { t.Run("should create PAT wildcards for allowed permissions", func(t *testing.T) { roleSvc := new(mockRoleService) @@ -290,3 +309,19 @@ func Test_migrateRole(t *testing.T) { roleSvc.AssertNotCalled(t, "Update") }) } + +func Test_AppendSchema(t *testing.T) { + t.Run("returns the error when listing existing permissions fails", func(t *testing.T) { + // A failed list must not be swallowed: boot would then apply an empty + // schema and drop every custom permission already in the database. + permSvc := new(mockPermissionService) + permSvc.On("List", mock.Anything, permission.Filter{}). + Return(nil, errors.New("db timeout")) + + svc := Service{permissionService: permSvc} + err := svc.AppendSchema(context.Background(), schema.ServiceDefinition{}) + + assert.Error(t, err) + assert.Contains(t, err.Error(), "db timeout") + }) +} From 462180bf015b7c5bf779cd5f64051a9f76d75f24 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Tue, 11 Aug 2026 14:30:39 +0530 Subject: [PATCH 2/3] fix(bootstrap): read permission description safely instead of panicking on a missing key --- internal/bootstrap/service.go | 23 ++++++++++++++++------- internal/bootstrap/service_test.go | 23 +++++++++++++++++++++++ 2 files changed, 39 insertions(+), 7 deletions(-) diff --git a/internal/bootstrap/service.go b/internal/bootstrap/service.go index 3ed9119288..314f9d5303 100644 --- a/internal/bootstrap/service.go +++ b/internal/bootstrap/service.go @@ -188,22 +188,31 @@ func (s Service) AppendSchema(ctx context.Context, customServiceDefinition schem return fmt.Errorf("AppendSchema: listing existing permissions: %w", err) } for _, existingPermission := range existingPermissions { - description := "" - if existingPermission.Metadata != nil { - if v, ok := existingPermission.Metadata["description"]; !ok { - description = v.(string) - } - } existingServiceDefinition.Permissions = append(existingServiceDefinition.Permissions, schema.ResourcePermission{ Name: existingPermission.Name, Namespace: existingPermission.NamespaceID, - Description: description, + Description: permissionDescription(existingPermission), }) } return s.applySchema(ctx, schema.MergeServiceDefinitions(customServiceDefinition, existingServiceDefinition)) } +// permissionDescription reads the human description out of a permission's +// metadata. It returns "" when the metadata is missing, has no description, or +// stores a non-string value, and never panics on a missing key. +func permissionDescription(p permission.Permission) string { + if p.Metadata == nil { + return "" + } + v, ok := p.Metadata["description"] + if !ok { + return "" + } + desc, _ := v.(string) + return desc +} + // applySchema builds and apply schema over az engine and db // schema is composed of inbuilt definitions and custom user defined services // this is idempotent operation and overrides existing schema diff --git a/internal/bootstrap/service_test.go b/internal/bootstrap/service_test.go index c4ebf88007..1f94d08e87 100644 --- a/internal/bootstrap/service_test.go +++ b/internal/bootstrap/service_test.go @@ -9,6 +9,7 @@ import ( "github.com/raystack/frontier/core/relation" "github.com/raystack/frontier/core/role" "github.com/raystack/frontier/internal/bootstrap/schema" + "github.com/raystack/frontier/pkg/metadata" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" ) @@ -325,3 +326,25 @@ func Test_AppendSchema(t *testing.T) { assert.Contains(t, err.Error(), "db timeout") }) } + +func Test_permissionDescription(t *testing.T) { + cases := []struct { + name string + meta metadata.Metadata + want string + }{ + {"nil metadata", nil, ""}, + // a present key must be read, not ignored + {"string description", metadata.Metadata{"description": "read access"}, "read access"}, + // a missing key must not panic; it used to assert nil to string + {"missing description key", metadata.Metadata{"other": "x"}, ""}, + // a non-string value must not panic either + {"non-string description", metadata.Metadata{"description": 42}, ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := permissionDescription(permission.Permission{Metadata: tc.meta}) + assert.Equal(t, tc.want, got) + }) + } +} From 9fa54032ec5fa9ee78f1f876029c2fe8250a669d Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Tue, 11 Aug 2026 15:36:32 +0530 Subject: [PATCH 3/3] refactor(bootstrap): simplify permissionDescription and cover the mapping --- internal/bootstrap/service.go | 45 +++++++++++++++--------------- internal/bootstrap/service_test.go | 20 +++++++++++-- 2 files changed, 40 insertions(+), 25 deletions(-) diff --git a/internal/bootstrap/service.go b/internal/bootstrap/service.go index 314f9d5303..b022eb87d9 100644 --- a/internal/bootstrap/service.go +++ b/internal/bootstrap/service.go @@ -16,6 +16,7 @@ import ( "github.com/raystack/frontier/core/relation" "github.com/raystack/frontier/core/role" "github.com/raystack/frontier/internal/bootstrap/schema" + "github.com/raystack/frontier/pkg/metadata" ) var ( @@ -179,37 +180,37 @@ func (s Service) BuiltinPermissions(ctx context.Context) (map[string]struct{}, e } func (s Service) AppendSchema(ctx context.Context, customServiceDefinition schema.ServiceDefinition) error { - // get existing permissions and append to the new definition - // this is required to avoid overriding existing permissions in authzed engine - var existingServiceDefinition schema.ServiceDefinition - + // re-apply the base schema merged with the permissions already in the + // database, so a re-apply never drops the existing ones. existingPermissions, err := s.permissionService.List(ctx, permission.Filter{}) if err != nil { return fmt.Errorf("AppendSchema: listing existing permissions: %w", err) } - for _, existingPermission := range existingPermissions { - existingServiceDefinition.Permissions = append(existingServiceDefinition.Permissions, schema.ResourcePermission{ - Name: existingPermission.Name, - Namespace: existingPermission.NamespaceID, - Description: permissionDescription(existingPermission), - }) - } + existingServiceDefinition := existingPermissionsAsServiceDefinition(existingPermissions) return s.applySchema(ctx, schema.MergeServiceDefinitions(customServiceDefinition, existingServiceDefinition)) } -// permissionDescription reads the human description out of a permission's -// metadata. It returns "" when the metadata is missing, has no description, or -// stores a non-string value, and never panics on a missing key. -func permissionDescription(p permission.Permission) string { - if p.Metadata == nil { - return "" - } - v, ok := p.Metadata["description"] - if !ok { - return "" +// existingPermissionsAsServiceDefinition maps the permissions already in the +// database into a service definition, so merging it into a re-applied schema +// keeps them. +func existingPermissionsAsServiceDefinition(perms []permission.Permission) schema.ServiceDefinition { + var def schema.ServiceDefinition + for _, p := range perms { + def.Permissions = append(def.Permissions, schema.ResourcePermission{ + Name: p.Name, + Namespace: p.NamespaceID, + Description: permissionDescription(p.Metadata), + }) } - desc, _ := v.(string) + return def +} + +// permissionDescription reads the human description out of a permission's +// metadata. Indexing a nil map and asserting a missing or non-string value are +// both safe, so this returns "" in those cases and never panics. +func permissionDescription(m metadata.Metadata) string { + desc, _ := m["description"].(string) return desc } diff --git a/internal/bootstrap/service_test.go b/internal/bootstrap/service_test.go index 1f94d08e87..78adb5531c 100644 --- a/internal/bootstrap/service_test.go +++ b/internal/bootstrap/service_test.go @@ -313,8 +313,8 @@ func Test_migrateRole(t *testing.T) { func Test_AppendSchema(t *testing.T) { t.Run("returns the error when listing existing permissions fails", func(t *testing.T) { - // A failed list must not be swallowed: boot would then apply an empty - // schema and drop every custom permission already in the database. + // The old code returned nil here, so a failed list skipped the schema + // re-apply but still reported boot success. Boot must surface the error. permSvc := new(mockPermissionService) permSvc.On("List", mock.Anything, permission.Filter{}). Return(nil, errors.New("db timeout")) @@ -327,6 +327,20 @@ func Test_AppendSchema(t *testing.T) { }) } +func Test_existingPermissionsAsServiceDefinition(t *testing.T) { + perms := []permission.Permission{ + {Name: "get", NamespaceID: "compute/order", Metadata: metadata.Metadata{"description": "read an order"}}, + {Name: "delete", NamespaceID: "compute/order"}, + } + + def := existingPermissionsAsServiceDefinition(perms) + + assert.Equal(t, []schema.ResourcePermission{ + {Name: "get", Namespace: "compute/order", Description: "read an order"}, + {Name: "delete", Namespace: "compute/order", Description: ""}, + }, def.Permissions) +} + func Test_permissionDescription(t *testing.T) { cases := []struct { name string @@ -343,7 +357,7 @@ func Test_permissionDescription(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - got := permissionDescription(permission.Permission{Metadata: tc.meta}) + got := permissionDescription(tc.meta) assert.Equal(t, tc.want, got) }) }