Repository: scimmyjs/scimmy (js)

Issue:
Egress list errors aren't catch and turned into SCIMMY errors
When egress isn't passed an id, it doesn't have a try catch converting any exceptions to the SCIMMY Error type. 

But it does when an id is passed.  It would be nicer to catch and return scim type errors in both cases: https://github.com/scimmyjs/scimmy/blob/8a25458fbc122e27d6b65cf52a91bdd0d140a62d/src/lib/resources/user.js#L86

Pull request:
Handle invalid or empty values returned by egress handlers in `SCIMMY.Resources.User` and `SCIMMY.Resources.Group`
Currently, the `read` methods of the `User` and `Group` resources will catch and throw SCIM errors when egress handlers return invalid or empty results, but only when the request is for a specific resource, and not a list of resources. When the return value of the egress handler is null or undefined, the request proceeds as if no resources matched the query.

This change wraps requests for a list of resources in the same catch and throw process as requests for specific resources, meaning invalid and empty values returned by egress handlers will always raise a 500 Internal Server Error response (fixes #43). Test fixtures have also been updated to verify that the exception is thrown for list requests and specific resource requests.

Patch:
diff --git a/src/lib/resources/group.js b/src/lib/resources/group.js
index 7ef66fe..f953797 100644
--- a/src/lib/resources/group.js
+++ b/src/lib/resources/group.js
@@ -82,19 +82,21 @@ export class Group extends Types.Resource {
      * await new SCIMMY.Resources.Group({filter: 'displayName sw "A"'}).read();
      */
     async read(ctx) {
-        if (!this.id) {
-            return new Messages.ListResponse((await Group.#egress(this, ctx) ?? [])
+        try {
+            const source = await Group.#egress(this, ctx);
+            const target = (this.id ? [source].flat().shift() : source);
+            
+            // If not looking for a specific resource, make sure egress returned an array
+            if (!this.id && Array.isArray(target)) return new Messages.ListResponse(target
                 .map(u => new Schemas.Group(u, "out", Group.basepath(), this.attributes)), this.constraints);
-        } else {
-            try {
-                const source = [await Group.#egress(this, ctx)].flat().shift();
-                if (!(source instanceof Object)) throw new Types.Error(500, null, `Unexpected ${source === undefined ? "empty" : "invalid"} value returned by handler`);
-                else return new Schemas.Group(source, "out", Group.basepath(), this.attributes);
-            } catch (ex) {
-                if (ex instanceof Types.Error) throw ex;
-                else if (ex instanceof TypeError) throw new Types.Error(400, "invalidValue", ex.message);
-                else throw new Types.Error(404, null, `Resource ${this.id} not found`);
-            }
+            // For specific resources, make sure egress returned an object
+            else if (target instanceof Object) return new Schemas.Group(target, "out", Group.basepath(), this.attributes);
+            // Otherwise, egress has not been implemented correctly
+            else throw new Types.Error(500, null, `Unexpected ${target === undefined ? "empty" : "invalid"} value returned by egress handler`);
+        } catch (ex) {
+            if (ex instanceof Types.Error) throw ex;
+            else if (ex instanceof TypeError) throw new Types.Error(400, "invalidValue", ex.message);
+            else throw new Types.Error(404, null, `Resource ${this.id} not found`);
         }
     }
     
@@ -116,8 +118,11 @@ export class Group extends Types.Resource {
         
         try {
             const target = await Group.#ingress(this, new Schemas.Group(instance, "in"), ctx);
-            if (!(target instanceof Object)) throw new Types.Error(500, null, `Unexpected ${target === undefined ? "empty" : "invalid"} value returned by handler`);
-            else return new Schemas.Group(target, "out", Group.basepath(), this.attributes);
+            
+            // Make sure ingress returned an object
+            if (target instanceof Object) return new Schemas.Group(target, "out", Group.basepath(), this.attributes);
+            // Otherwise, ingress has not been implemented correctly
+            else throw new Types.Error(500, null, `Unexpected ${target === undefined ? "empty" : "invalid"} value returned by ingress handler`);
         } catch (ex) {
             if (ex instanceof Types.Error) throw ex;
             else if (ex instanceof TypeError) throw new Types.Error(400, "invalidValue", ex.message);
diff --git a/src/lib/resources/user.js b/src/lib/resources/user.js
index 6f8baa1..1883df5 100644
--- a/src/lib/resources/user.js
+++ b/src/lib/resources/user.js
@@ -82,19 +82,21 @@ export class User extends Types.Resource {
      * await new SCIMMY.Resources.User({filter: 'email.value ew "@example.com"'}).read();
      */
     async read(ctx) {
-        if (!this.id) {
-            return new Messages.ListResponse((await User.#egress(this, ctx) ?? [])
+        try {
+            const source = await User.#egress(this, ctx);
+            const target = (this.id ? [source].flat().shift() : source);
+            
+            // If not looking for a specific resource, make sure egress returned an array
+            if (!this.id && Array.isArray(target)) return new Messages.ListResponse(target
                 .map(u => new Schemas.User(u, "out", User.basepath(), this.attributes)), this.constraints);
-        } else {
-            try {
-                const source = [await User.#egress(this, ctx)].flat().shift();
-                if (!(source instanceof Object)) throw new Types.Error(500, null, `Unexpected ${source === undefined ? "empty" : "invalid"} value returned by handler`);
-                else return new Schemas.User(source, "out", User.basepath(), this.attributes);
-            } catch (ex) {
-                if (ex instanceof Types.Error) throw ex;
-                else if (ex instanceof TypeError) throw new Types.Error(400, "invalidValue", ex.message);
-                else throw new Types.Error(404, null, `Resource ${this.id} not found`);
-            }
+            // For specific resources, make sure egress returned an object
+            else if (target instanceof Object) return new Schemas.User(target, "out", User.basepath(), this.attributes);
+            // Otherwise, egress has not been implemented correctly
+            else throw new Types.Error(500, null, `Unexpected ${target === undefined ? "empty" : "invalid"} value returned by egress handler`);
+        } catch (ex) {
+            if (ex instanceof Types.Error) throw ex;
+            else if (ex instanceof TypeError) throw new Types.Error(400, "invalidValue", ex.message);
+            else throw new Types.Error(404, null, `Resource ${this.id} not found`);
         }
     }
     
@@ -116,8 +118,11 @@ export class User extends Types.Resource {
         
         try {
             const target = await User.#ingress(this, new Schemas.User(instance, "in"), ctx);
-            if (!(target instanceof Object)) throw new Types.Error(500, null, `Unexpected ${target === undefined ? "empty" : "invalid"} value returned by handler`);
-            else return new Schemas.User(target, "out", User.basepath(), this.attributes);
+            
+            // Make sure ingress returned an object
+            if (target instanceof Object) return new Schemas.User(target, "out", User.basepath(), this.attributes);
+            // Otherwise, ingress has not been implemented correctly
+            else throw new Types.Error(500, null, `Unexpected ${target === undefined ? "empty" : "invalid"} value returned by ingress handler`);
         } catch (ex) {
             if (ex instanceof Types.Error) throw ex;
             else if (ex instanceof TypeError) throw new Types.Error(400, "invalidValue", ex.message);


Test patch:
diff --git a/test/hooks/resources.js b/test/hooks/resources.js
index 1497adc..e50a119 100644
--- a/test/hooks/resources.js
+++ b/test/hooks/resources.js
@@ -423,28 +423,32 @@ export default class ResourcesHooks {
             
             if (callsEgress) {
                 (skip ? it.skip : it)("should throw exception for invalid values returned by handler", async () => {
-                    handler.reset();
-                    
                     for (let value of [true, false, "invalid", null]) {
+                        handler.reset();
                         handler.returns(value);
                         
-                        await assert.rejects(() => new TargetResource("placeholder").read(),
+                        for (let id of [undefined, "placeholder"]) await assert.rejects(() => new TargetResource("placeholder").read(),
                             {name: "SCIMError", status: 500, scimType: null,
-                                message: "Unexpected invalid value returned by handler"},
-                            "Instance method 'read' did not throw exception for invalid values returned by handler");
+                                message: "Unexpected invalid value returned by egress handler"},
+                            `Instance method 'read' (${id ? "one" : "many"}) did not throw exception for invalid value '${JSON.stringify(value)}' returned by handler`);
                     }
                 });
                 
                 (skip ? it.skip : it)("should throw exception for empty values returned by handler", async () => {
-                    handler.reset();
+                    const fixtures = [
+                        [undefined, undefined], 
+                        [undefined, "placeholder"], 
+                        [[], "placeholder"]
+                    ];
                     
-                    for (let value of [undefined, []]) {
+                    for (let [value, id] of fixtures) {
+                        handler.reset();
                         handler.returns(value);
                         
-                        await assert.rejects(() => new TargetResource("placeholder").read(),
+                        await assert.rejects(() => new TargetResource(id).read(),
                             {name: "SCIMError", status: 500, scimType: null,
-                                message: "Unexpected empty value returned by handler"},
-                            "Instance method 'read' did not throw exception for empty values returned by handler");
+                                message: "Unexpected empty value returned by egress handler"},
+                            `Instance method 'read' (${id ? "one" : "many"}) did not throw exception for empty value '${JSON.stringify(value)}' returned by handler`);
                     }
                 });
                 
@@ -589,7 +593,7 @@ export default class ResourcesHooks {
                 
                 await assert.rejects(() => new TargetResource().write(source),
                     {name: "SCIMError", status: 500, scimType: null,
-                        message: "Unexpected invalid value returned by handler"},
+                        message: "Unexpected invalid value returned by ingress handler"},
                     "Instance method 'write' did not throw exception for invalid values returned by handler");
             });
             
@@ -601,7 +605,7 @@ export default class ResourcesHooks {
                 
                 await assert.rejects(() => new TargetResource().write(source),
                     {name: "SCIMError", status: 500, scimType: null,
-                        message: "Unexpected empty value returned by handler"},
+                        message: "Unexpected empty value returned by ingress handler"},
                     "Instance method 'write' did not throw exception for empty values returned by handler");
             });
             
@@ -797,7 +801,7 @@ export default class ResourcesHooks {
                     
                     await assert.rejects(() => new TargetResource(fixture.id).patch(message),
                         {name: "SCIMError", status: 500, scimType: null,
-                            message: "Unexpected empty value returned by handler"},
+                            message: `Unexpected empty value returned by ${method} handler`},
                         `Instance method 'patch' did not rethrow exception for empty values returned by ${method} handler`);
                 });