improved error and status code handling

src/middlewares/openapi.security.ts

@@ -5,6 +5,7 @@ import { OpenApiContext } from '../framework/openapi.context';
 
 interface SecurityHandlerResult {
   success: boolean;
+  status: number;
   error: string;
 }
 export function security(
@@ -46,18 +47,18 @@ export function security(
 
       // TODO handle AND'd and OR'd security
       // This assumes OR only! i.e. at least one security passed authentication
-      let firstError = null;
+      let firstError: SecurityHandlerResult = null;
       for (var r of results) {
         if (!r.success) {
-          firstError = r.error || Error('unauthorized');
+          firstError = r;
           break;
         }
       }
       if (firstError) throw firstError;
       else next();
     } catch (e) {
-      const message = (e && e.message) || 'unauthorized';
-      const err = validationError(401, path, message);
+      const message = (e && e.error && e.error.message) || 'unauthorized';
+      const err = validationError(e.status, path, message);
       next(err);
     }
   };
@@ -84,25 +85,36 @@ class SecuritySchemes {
 
         if (!scheme) {
           const message = `components.securitySchemes.${securityKey} does not exist`;
-          throw Error(message);
+          throw { status: 500, message };
         }
         if (!scheme.type) {
           const message = `components.securitySchemes.${securityKey} must have property 'type'`;
-          throw Error(message);
+          throw { status: 500, message };
         }
-        // TODO fail on invalid scheme.type
         if (!handler) {
-          const message = `a handler for ${securityKey} does not exist`;
-          throw Error(message);
+          const message = `a handler for '${securityKey}' does not exist`;
+          throw { status: 500, message };
         }
 
         new AuthValidator(req, scheme).validate();
 
+        // expected handler results are:
+        // - throw exception, 
+        // - return true, 
+        // - return Promise<true>, 
+        // - return false, 
+        // - return Promise<false>
+        // everything else should be treated as false
         const success = await handler(req, scopes, scheme);
-        return { success };
+        if (success === true) {
+          return { success };
+        } else {
+          throw Error();
+        }
       } catch (e) {
         return {
           success: false,
+          status: e.status || 401,
           error: e,
         };
       }

test/security.spec.ts

@@ -7,14 +7,16 @@ import { config } from 'chai/lib/chai';
 
 const packageJson = require('../package.json');
 
+// NOTE/TODO: These tests modify eovConf.securityHandlers
+// Thus test execution order matters :-(
 describe(packageJson.name, () => {
   let app = null;
   let basePath = null;
   const eovConf = {
     apiSpec: path.join('test', 'resources', 'security.yaml'),
     securityHandlers: {
       ApiKeyAuth: function(req, scopes, schema) {
-        throw { errors: [] };
+        throw Error('custom api key handler failed');
       },
     },
   };
@@ -48,7 +50,9 @@ describe(packageJson.name, () => {
         const body = r.body;
         expect(body.errors).to.be.an('array');
         expect(body.errors).to.have.length(1);
-        expect(body.errors[0].message).to.equals('unauthorized');
+        expect(body.errors[0].message).to.equals(
+          'custom api key handler failed',
+        );
       }));
 
   it('should return 401 if apikey handler returns false', async () => {
@@ -242,4 +246,27 @@ describe(packageJson.name, () => {
       .get(`${basePath}/openid`)
       .expect(200);
   });
+
+  it('should return 500 if missing handler', async () => {
+    delete (<any>eovConf.securityHandlers).OpenID;
+    (<any>eovConf.securityHandlers).Test = <any>function(req, scopes, schema) {
+      expect(schema.type).to.equal('openIdConnect');
+      expect(schema).to.have.property('openIdConnectUrl');
+      expect(scopes)
+        .to.be.an('array')
+        .with.length(2);
+
+      return true;
+    };
+    return request(app)
+      .get(`${basePath}/openid`)
+      .expect(500)
+      .then(r => {
+        const body = r.body;
+        const msg = "a handler for 'OpenID' does not exist";
+        expect(body.message).to.equal(msg);
+        expect(body.errors[0].message).to.equal(msg);
+        expect(body.errors[0].path).to.equal(`${basePath}/openid`);
+      });
+  });
 });