fix: correct security schema logic for OR verification (#946)

Author: lukebellamy053

Diff for: src/middlewares/openapi.security.ts

@@ -1,11 +1,11 @@
 import {
-  OpenAPIV3,
+  HttpError,
+  InternalServerError,
   OpenApiRequest,
-  SecurityHandlers,
-  OpenApiRequestMetadata,
   OpenApiRequestHandler,
-  InternalServerError,
-  HttpError,
+  OpenApiRequestMetadata,
+  OpenAPIV3,
+  SecurityHandlers,
 } from '../framework/types';
 
 const defaultSecurityHandler = (
@@ -17,11 +17,30 @@ const defaultSecurityHandler = (
 type SecuritySchemesMap = {
   [key: string]: OpenAPIV3.ReferenceObject | OpenAPIV3.SecuritySchemeObject;
 };
+
 interface SecurityHandlerResult {
   success: boolean;
   status?: number;
   error?: string;
 }
+
+function extractErrorsFromResults(results: (SecurityHandlerResult | SecurityHandlerResult[])[]) {
+  return results.map(result => {
+    if (Array.isArray(result)) {
+      return result.map(it => it).filter(it => !it.success);
+    }
+    return [result].filter(it => !it.success);
+  }).flatMap(it => [...it]);
+}
+
+function didAllSecurityRequirementsPass(results: SecurityHandlerResult[]) {
+  return results.every(it => it.success);
+}
+
+function didOneSchemaPassValidation(results: (SecurityHandlerResult | SecurityHandlerResult[])[]) {
+  return results.some(result => Array.isArray(result) ? didAllSecurityRequirementsPass(result) : result.success);
+}
+
 export function security(
   apiDoc: OpenAPIV3.Document,
   securityHandlers: SecurityHandlers,
@@ -62,50 +81,13 @@ 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;
-
-      function checkErrorWithOr(res) {
-        return res.forEach((r) => {
-          if (r.success) {
-            success = true;
-          } else if (!firstError) {
-            firstError = r;
-          }
-        });
-      }
-
-      function checkErrorsWithAnd(res) {
-        let allSuccess = false;
-
-        res.forEach((r) => {
-          if (!r.success) {
-            allSuccess = false;
-            if (!firstError) {
-              firstError = r;
-            }
-          } else if (!firstError) {
-            allSuccess = true;
-          }
-        });
-
-        if (allSuccess) {
-          success = true;
-        }
-      }
-
-      results.forEach((result) => {
-        if (Array.isArray(result) && result.length > 1) {
-          checkErrorsWithAnd(result);
-        } else {
-          checkErrorWithOr(result);
-        }
-      });
+      const success = didOneSchemaPassValidation(results);
 
       if (success) {
         next();
       } else {
-        throw firstError;
+        const errors = extractErrorsFromResults(results)
+        throw errors[0]
       }
     } catch (e) {
       const message = e?.error?.message || 'unauthorized';
@@ -129,6 +111,7 @@ class SecuritySchemes {
   private securitySchemes: SecuritySchemesMap;
   private securityHandlers: SecurityHandlers;
   private securities: OpenAPIV3.SecurityRequirementObject[];
+
   constructor(
     securitySchemes: SecuritySchemesMap,
     securityHandlers: SecurityHandlers,
@@ -213,6 +196,7 @@ class AuthValidator {
   private scheme;
   private path: string;
   private scopes: string[];
+
   constructor(req: OpenApiRequest, scheme, scopes: string[] = []) {
     const openapi = <OpenApiRequestMetadata>req.openapi;
     this.req = req;

Diff for: test/resources/security.yaml

@@ -90,6 +90,19 @@ paths:
         "401":
           description: unauthorized
 
+  /multi_auth:
+    get:
+      security:
+        - ApiKeyAuth: []
+          BearerAuth: []
+        - BasicAuth: []
+          CookieAuth: []
+      responses:
+        "200":
+          description: OK
+        "401":
+          description: unauthorized
+
   /oauth2:
     get:
       security:

Diff for: test/security.defaults.spec.ts

@@ -21,7 +21,8 @@ describe('security.defaults', () => {
         .get(`/cookie_auth`, (req, res) => res.json({ logged_in: true }))
         .get(`/bearer`, (req, res) => res.json({ logged_in: true }))
         .get(`/basic`, (req, res) => res.json({ logged_in: true }))
-        .get('/no_security', (req, res) => res.json({ logged_in: true })),
+        .get('/no_security', (req, res) => res.json({ logged_in: true }))
+        .get('/multi_auth', (req, res) => res.json({ logged_in: true }))
     );
   });
 
@@ -64,4 +65,33 @@ describe('security.defaults', () => {
           .that.equals('cookie \'JSESSIONID\' required');
       });
   });
+
+  it("Should return 200 WHEN one of multiple security rules is met GIVEN a request with apiKey and bearer token", () => {
+    return request(app)
+      .get(`${basePath}/multi_auth`)
+      .set({
+        "Authorization": "Bearer rawww",
+        "X-API-Key": "hello world"
+      })
+      .expect(200)
+  })
+
+  it("Should return 200 WHEN one of multiple security rules is met GIVEN a request with cookie and basic auth", () => {
+    return request(app)
+      .get(`${basePath}/multi_auth`)
+      .set({
+        "Authorization": "Basic ZGVtbzpwQDU1dzByZA==",
+        "Cookie": "JSESSIONID=dwdwdwdwd"
+      })
+      .expect(200)
+  })
+
+  it("Should return 401 WHEN none of multiple security rules is met GIVEN a request with only cookie auth", () => {
+    return request(app)
+      .get(`${basePath}/multi_auth`)
+      .set({
+        "Cookie": "JSESSIONID=dwdwdwdwd"
+      })
+      .expect(401)
+  })
 });