diff --git a/internal/bootstrap/service.go b/internal/bootstrap/service.go index a4d9dd26b0..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,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 diff --git a/internal/bootstrap/service_test.go b/internal/bootstrap/service_test.go index 28c1f78c58..78adb5531c 100644 --- a/internal/bootstrap/service_test.go +++ b/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) + }) + } +}