Merge pull request #69 from cdimascio/security

cdimascio · web-flow · commit 8b406899769d · 2019-10-13T22:39:06.000-04:00
security improvements
diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "express-openapi-validator",
-  "version": "2.6.1",
+  "version": "2.7.0",
   "description": "Automatically validate API requests using an OpenAPI 3 and Express.",
   "main": "dist/index.js",
   "scripts": {
diff --git a/src/framework/openapi.spec.loader.ts b/src/framework/openapi.spec.loader.ts
@@ -44,7 +44,6 @@ export class OpenApiSpecLoader {
     framework.initialize({
       visitApi(ctx: OpenAPIFrameworkAPIContext) {
         const apiDoc = ctx.getApiDoc();
-        const security = apiDoc.security;
         for (const bpa of basePaths) {
           const bp = bpa.replace(/\/$/, '');
           for (const [path, methods] of Object.entries(apiDoc.paths)) {
@@ -72,23 +71,12 @@ export class OpenApiSpecLoader {
                 .map(toExpressParams)
                 .join('/');
 
-              // add apply any general defined security 
-              const moddedSchema =
-                security || schema.security
-                  ? {
-                      schema,
-                      security: [
-                        ...(security || []),
-                        ...(schema.security || []),
-                      ],
-                    }
-                  : { ...schema };
               routes.push({
                 expressRoute,
                 openApiRoute,
                 method: method.toUpperCase(),
                 pathParams: Array.from(pathParams),
-                schema: moddedSchema,
+                schema,
               });
             }
           }
diff --git a/src/middlewares/openapi.security.ts b/src/middlewares/openapi.security.ts
@@ -11,8 +11,8 @@ const defaultSecurityHandler = (
 
 interface SecurityHandlerResult {
   success: boolean;
-  status: number;
-  error: string;
+  status?: number;
+  error?: string;
 }
 export function security(
   context: OpenApiContext,
@@ -26,13 +26,16 @@ export function security(
       return next();
     }
 
-    const securities = <OpenAPIV3.SecuritySchemeObject>(
-      req.openapi.schema.security
-    );
+    // const securities: OpenAPIV3.SecurityRequirementObject[] =
+    //   req.openapi.schema.security;
+
+    // use the local security object or fallbac to api doc's security or undefined
+    const securities: OpenAPIV3.SecurityRequirementObject[] =
+      req.openapi.schema.security || context.apiDoc.security;
 
     const path: string = req.openapi.openApiRoute;
 
-    if (!path || !Array.isArray(securities)) {
+    if (!path || !Array.isArray(securities) || securities.length === 0) {
       return next();
     }
 
@@ -54,14 +57,17 @@ 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: SecurityHandlerResult = null;
+      let success = false;
       for (var r of results) {
-        if (!r.success) {
-          firstError = r;
+        if (r.success) {
+          success = true;
           break;
+        } else if (!firstError) {
+          firstError = r;
         }
       }
-      if (firstError) throw firstError;
-      else next();
+      if (success) next();
+      else throw firstError;
     } catch (e) {
       const message = (e && e.error && e.error.message) || 'unauthorized';
       const err = validationError(e.status, path, message);
@@ -80,7 +86,7 @@ class SecuritySchemes {
     this.securities = securities;
   }
 
-  executeHandlers(req: OpenApiRequest): Promise<SecurityHandlerResult[]> {
+  async executeHandlers(req: OpenApiRequest): Promise<SecurityHandlerResult[]> {
     // use a fallback handler if security handlers is not specified
     // This means if security handlers is specified, the user must define
     // all security handlers
@@ -90,9 +96,15 @@ class SecuritySchemes {
 
     const promises = this.securities.map(async s => {
       try {
+        if (Util.isEmptyObject(s)) {
+          // anonumous security
+          return { success: true };
+        }
         const securityKey = Object.keys(s)[0];
         const scheme: any = this.securitySchemes[securityKey];
-        const handler = this.securityHandlers[securityKey] || fallbackHandler;
+        const handler =
+          (this.securityHandlers && this.securityHandlers[securityKey]) ||
+          fallbackHandler;
         const scopesTmp = s[securityKey];
         const scopes = Array.isArray(scopesTmp) ? scopesTmp : [];
 
@@ -109,7 +121,7 @@ class SecuritySchemes {
           throw { status: 500, message };
         }
 
-        new AuthValidator(req, scheme).validate();
+        new AuthValidator(req, scheme, scopes).validate();
 
         // expected handler results are:
         // - throw exception,
@@ -140,10 +152,12 @@ class AuthValidator {
   private req: OpenApiRequest;
   private scheme;
   private path: string;
-  constructor(req: OpenApiRequest, scheme) {
+  private scopes: string[];
+  constructor(req: OpenApiRequest, scheme, scopes: string[] = []) {
     this.req = req;
     this.scheme = scheme;
     this.path = req.openapi.openApiRoute;
+    this.scopes = scopes;
   }
 
   validate() {
@@ -186,6 +200,8 @@ class AuthValidator {
       if (type === 'basic' && !authHeader.includes('basic')) {
         throw Error(`Authorization header with scheme 'Basic' required.`);
       }
+
+      this.dissallowScopes();
     }
   }
 
@@ -202,6 +218,28 @@ class AuthValidator {
         }
       }
       // TODO scheme in cookie
+
+      this.dissallowScopes();
     }
   }
+
+  private dissallowScopes() {
+    if (this.scopes.length > 0) {
+      // https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#security-requirement-object
+      throw {
+        status: 500,
+        message: "scopes array must be empty for security type 'http'",
+      };
+    }
+  }
+}
+
+class Util {
+  static isEmptyObject(o: Object) {
+    return (
+      typeof o === 'object' &&
+      Object.entries(o).length === 0 &&
+      o.constructor === Object
+    );
+  }
 }
diff --git a/test/resources/security.top.level.yaml b/test/resources/security.top.level.yaml
@@ -0,0 +1,69 @@
+openapi: '3.0.2'
+info:
+  version: 1.0.0
+  title: security top level
+  description: security top level
+
+servers:
+  - url: /v1/
+
+security:
+  - ApiKeyAuth: []
+
+paths:
+  /api_key:
+    get:
+      responses:
+        '200':
+          description: OK
+        '401':
+          description: unauthorized
+
+  /api_key_or_anonymous:
+    get:
+      security:
+        - ApiKeyAuth: []
+        - {}
+      responses:
+        '200':
+          description: OK
+        '401':
+          description: unauthorized
+
+  /bearer:
+    get:
+      security:
+        - BearerAuth: []
+      responses:
+        '200':
+          description: OK
+        '401':
+          description: unauthorized
+
+  /anonymous:
+    get:
+      security: []
+      responses:
+        '200':
+          description: OK
+        '401':
+          description: unauthorized
+
+  /anonymous_2:
+    get:
+      security:
+        - {}
+      responses:
+        '200':
+          description: OK
+        '401':
+          description: unauthorized
+components:
+  securitySchemes:
+    ApiKeyAuth:
+      type: apiKey
+      in: header
+      name: X-API-Key
+    BearerAuth:
+      type: http
+      scheme: bearer
diff --git a/test/resources/security.yaml b/test/resources/security.yaml
@@ -8,15 +8,43 @@ servers:
   - url: /v1/
 
 paths:
+  /no_security: 
+    get:
+      responses:
+        '200':
+          description: OK
+
   /api_key:
     get:
       security:
         - ApiKeyAuth: []
       responses:
         '200':
           description: OK
-        '400':
-          description: Bad Request
+        '401':
+          description: unauthorized
+
+  /api_key_or_anonymous:
+    get:
+      security:
+        # {} means anonyous or no security - see https://github.com/OAI/OpenAPI-Specification/issues/14
+        - {}
+        - ApiKeyAuth: []
+      responses:
+        '200':
+          description: OK
+        '401':
+          description: unauthorized
+
+  # This api key with scopes should fail validation and return 500
+  # scopes are only allowed for oauth2 and openidconnect
+  /api_key_with_scopes:
+    get:
+      security:
+        - ApiKeyAuth: ['read', 'write']
+      responses:
+        '200':
+          description: OK
         '401':
           description: unauthorized
 
diff --git a/test/security.spec.ts b/test/security.spec.ts
@@ -33,14 +33,23 @@ describe(packageJson.name, () => {
         .get(`/bearer`, (req, res) => res.json({ logged_in: true }))
         .get(`/basic`, (req, res) => res.json({ logged_in: true }))
         .get(`/oauth2`, (req, res) => res.json({ logged_in: true }))
-        .get(`/openid`, (req, res) => res.json({ logged_in: true })),
+        .get(`/openid`, (req, res) => res.json({ logged_in: true }))
+        .get(`/api_key_or_anonymous`, (req, res) =>
+          res.json({ logged_in: true }),
+        )
+        .get('/no_security', (req, res) => res.json({ logged_in: true })),
     );
   });
 
   after(() => {
     app.server.close();
   });
 
+  it('should return 200 if no security', async () =>
+    request(app)
+      .get(`${basePath}/no_security`)
+      .expect(200));
+
   it('should return 401 if apikey handler throws exception', async () =>
     request(app)
       .get(`${basePath}/api_key`)
@@ -309,4 +318,35 @@ describe(packageJson.name, () => {
         expect(body.errors[0].path).to.equal(`${basePath}/openid`);
       });
   });
+
+  it('should return 500 if scopes are no allowed', async () =>
+    request(app)
+      .get(`${basePath}/api_key_with_scopes`)
+      .set('X-Api-Key', 'XXX')
+      .expect(500)
+      .then(r => {
+        const body = r.body;
+        expect(body.message).to.equal(
+          "scopes array must be empty for security type 'http'",
+        );
+      }));
+
+  it('should return 200 if api_key or anonymous and no api key is supplied', async () => {
+    (<any>eovConf.securityHandlers).ApiKeyAuth = <any>(
+      ((req, scopes, schema) => true)
+    );
+    return request(app)
+      .get(`${basePath}/api_key_or_anonymous`)
+      .expect(200);
+  });
+
+  it('should return 200 if api_key or anonymous and api key is supplied', async () => {
+    (<any>eovConf.securityHandlers).ApiKeyAuth = <any>(
+      ((req, scopes, schema) => true)
+    );
+    return request(app)
+      .get(`${basePath}/api_key_or_anonymous`)
+      .set('x-api-key', 'XXX')
+      .expect(200);
+  });
 });
diff --git a/test/security.top.level.spec.ts b/test/security.top.level.spec.ts

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "express-openapi-validator",`
`3`		`- "version": "2.6.1",`
	`3`	`+ "version": "2.7.0",`
`4`	`4`	`"description": "Automatically validate API requests using an OpenAPI 3 and Express.",`
`5`	`5`	`"main": "dist/index.js",`
`6`	`6`	`"scripts": {`