From fbd9cbb249c6b5aa298e3c4ee399bf51f4417545 Mon Sep 17 00:00:00 2001 From: Carmine DiMascio Date: Sun, 13 Oct 2019 22:34:39 -0400 Subject: [PATCH] add support for OR securities and local overrides --- src/framework/openapi.spec.loader.ts | 14 +---- src/middlewares/openapi.security.ts | 62 ++++++++++++++---- test/resources/security.top.level.yaml | 69 ++++++++++++++++++++ test/resources/security.yaml | 32 +++++++++- test/security.spec.ts | 42 ++++++++++++- test/security.top.level.spec.ts | 87 ++++++++++++++++++++++++++ 6 files changed, 277 insertions(+), 29 deletions(-) create mode 100644 test/resources/security.top.level.yaml create mode 100644 test/security.top.level.spec.ts diff --git a/src/framework/openapi.spec.loader.ts b/src/framework/openapi.spec.loader.ts index 5a271124..0e2d2741 100644 --- 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 index b828c025..5d5b9e86 100644 --- 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 = ( - 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 { + async executeHandlers(req: OpenApiRequest): Promise { // 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,6 +96,10 @@ 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 = @@ -111,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, @@ -142,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() { @@ -188,6 +200,8 @@ class AuthValidator { if (type === 'basic' && !authHeader.includes('basic')) { throw Error(`Authorization header with scheme 'Basic' required.`); } + + this.dissallowScopes(); } } @@ -204,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 new file mode 100644 index 00000000..5ad6a831 --- /dev/null +++ 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 index 550f3c90..1482e63e 100644 --- a/test/resources/security.yaml +++ b/test/resources/security.yaml @@ -8,6 +8,12 @@ servers: - url: /v1/ paths: + /no_security: + get: + responses: + '200': + description: OK + /api_key: get: security: @@ -15,8 +21,30 @@ paths: 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 index 8dd59e9e..e7014978 100644 --- a/test/security.spec.ts +++ b/test/security.spec.ts @@ -33,7 +33,11 @@ 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 })), ); }); @@ -41,6 +45,11 @@ describe(packageJson.name, () => { 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 () => { + (eovConf.securityHandlers).ApiKeyAuth = ( + ((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 () => { + (eovConf.securityHandlers).ApiKeyAuth = ( + ((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 new file mode 100644 index 00000000..3d222211 --- /dev/null +++ b/test/security.top.level.spec.ts @@ -0,0 +1,87 @@ +import * as path from 'path'; +import * as express from 'express'; +import { expect } from 'chai'; +import * as request from 'supertest'; +import { createApp } from './common/app'; + +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.top.level.yaml'), + }; + + before(async () => { + // Set up the express app + app = await createApp(eovConf, 3005); + basePath = app.basePath; + + app.use( + `${basePath}`, + express + .Router() + .get(`/api_key`, (req, res) => res.json({ logged_in: true })) + .get(`/api_key_or_anonymous`, (req, res) => + res.json({ logged_in: true }), + ) + .get('/anonymous', (req, res) => res.json({ logged_in: true })) + .get('/anonymous_2', (req, res) => res.json({ logged_in: true })) + .get(`/bearer`, (req, res) => res.json({ logged_in: true })), + ); + }); + + after(() => { + app.server.close(); + }); + + it('should inherit top level security and return 401 if apikey header is missing', async () => + request(app) + .get(`${basePath}/api_key`) + .expect(401) + .then(r => { + 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( + "'X-API-Key' header required.", + ); + })); + + it('should return 200 if apikey or anonymous', async () => + request(app) + .get(`${basePath}/api_key_or_anonymous`) + .expect(200)); + + it('should override api key with bearer and return 401 if bearer is missing', async () => + request(app) + .get(`${basePath}/bearer`) + .expect(401) + .then(r => { + 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( + 'Authorization header required.', + ); + })); + + it('should override api key with bearer and return 200', async () => + request(app) + .get(`${basePath}/bearer`) + .set('Authorization', 'Bearer XXX') + .expect(200)); + + it('should override api key with anonymous', async () => + request(app) + .get(`${basePath}/anonymous_2`) + .expect(200)); + + it('should override api key with anonymous', async () => + request(app) + .get(`${basePath}/anonymous`) + .expect(200)); +});