fix(bootstrap): harden AppendSchema against list errors and missing metadata description

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,29 +180,38 @@ 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 nil
+		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,
+	existingServiceDefinition := existingPermissionsAsServiceDefinition(existingPermissions)
+
+	return s.applySchema(ctx, schema.MergeServiceDefinitions(customServiceDefinition, existingServiceDefinition))
+}
+
+// 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),
 		})
 	}
+	return def
+}
 
-	return s.applySchema(ctx, schema.MergeServiceDefinitions(customServiceDefinition, existingServiceDefinition))
+// 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
 }
 
 // applySchema builds and apply schema over az engine and db

internal/bootstrap/service_test.go

@@ -5,9 +5,11 @@ 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"
+	"github.com/raystack/frontier/pkg/metadata"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 )
@@ -55,6 +57,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 +310,55 @@ 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) {
+		// 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"))
+
+		svc := Service{permissionService: permSvc}
+		err := svc.AppendSchema(context.Background(), schema.ServiceDefinition{})
+
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), "db timeout")
+	})
+}
+
+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
+		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(tc.meta)
+			assert.Equal(t, tc.want, got)
+		})
+	}
+}