From fb2a1543f59cc4de0dd1bf1aea4ed2f9fe527a06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20=5BDesktop=5D?= Date: Fri, 8 Sep 2017 13:22:39 +0200 Subject: [PATCH] Fix sync and async auth checker bug --- src/driver/express/ExpressDriver.ts | 34 +++-- src/driver/koa/KoaDriver.ts | 38 ++--- test/functional/auth-decorator.spec.ts | 195 +++++++++++++++++++------ 3 files changed, 189 insertions(+), 78 deletions(-) diff --git a/src/driver/express/ExpressDriver.ts b/src/driver/express/ExpressDriver.ts index afbb10b5..12ddb664 100644 --- a/src/driver/express/ExpressDriver.ts +++ b/src/driver/express/ExpressDriver.ts @@ -103,23 +103,27 @@ export class ExpressDriver extends BaseDriver implements Driver { throw new AuthorizationCheckerNotDefinedError(); const action: Action = { request, response, next }; - const checkResult = this.authorizationChecker(action, actionMetadata.authorizedRoles); - - const handleError = (result: any) => { - if (!result) { - let error = actionMetadata.authorizedRoles.length === 0 ? new AuthorizationRequiredError(action) : new AccessDeniedError(action); - return this.handleError(error, actionMetadata, action); + try { + const checkResult = this.authorizationChecker(action, actionMetadata.authorizedRoles); + + const handleError = (result: any) => { + if (!result) { + let error = actionMetadata.authorizedRoles.length === 0 ? new AuthorizationRequiredError(action) : new AccessDeniedError(action); + this.handleError(error, actionMetadata, action); + } else { + next(); + } + }; + + if (isPromiseLike(checkResult)) { + checkResult + .then(result => handleError(result)) + .catch(error => this.handleError(error, actionMetadata, action)); } else { - next(); + handleError(checkResult); } - }; - - if (isPromiseLike(checkResult)) { - checkResult - .then(result => handleError(result)) - .catch(error => this.handleError(error, actionMetadata, action)); - } else { - handleError(checkResult); + } catch (error) { + this.handleError(error, actionMetadata, action); } }); } diff --git a/src/driver/koa/KoaDriver.ts b/src/driver/koa/KoaDriver.ts index 40b48cd6..e92ffe6e 100644 --- a/src/driver/koa/KoaDriver.ts +++ b/src/driver/koa/KoaDriver.ts @@ -77,25 +77,29 @@ export class KoaDriver extends BaseDriver implements Driver { throw new AuthorizationCheckerNotDefinedError(); const action: Action = { request: context.request, response: context.response, context, next }; - const checkResult = actionMetadata.authorizedRoles instanceof Function ? - getFromContainer(actionMetadata.authorizedRoles).check(action) : - this.authorizationChecker(action, actionMetadata.authorizedRoles); - - const handleError = (result: any) => { - if (!result) { - let error = actionMetadata.authorizedRoles.length === 0 ? new AuthorizationRequiredError(action) : new AccessDeniedError(action); - return this.handleError(error, actionMetadata, action); + try { + const checkResult = actionMetadata.authorizedRoles instanceof Function ? + getFromContainer(actionMetadata.authorizedRoles).check(action) : + this.authorizationChecker(action, actionMetadata.authorizedRoles); + + const handleError = (result: any) => { + if (!result) { + let error = actionMetadata.authorizedRoles.length === 0 ? new AuthorizationRequiredError(action) : new AccessDeniedError(action); + return this.handleError(error, actionMetadata, action); + } else { + return next(); + } + }; + + if (isPromiseLike(checkResult)) { + return checkResult + .then(result => handleError(result)) + .catch(error => this.handleError(error, actionMetadata, action)); } else { - next(); + return handleError(checkResult); } - }; - - if (isPromiseLike(checkResult)) { - return checkResult - .then(result => handleError(result)) - .catch(error => this.handleError(error, actionMetadata, action)); - } else { - handleError(checkResult); + } catch (error) { + return this.handleError(error, actionMetadata, action); } }); } diff --git a/test/functional/auth-decorator.spec.ts b/test/functional/auth-decorator.spec.ts index 5660c268..b305b5a6 100644 --- a/test/functional/auth-decorator.spec.ts +++ b/test/functional/auth-decorator.spec.ts @@ -1,6 +1,6 @@ import "reflect-metadata"; import {Get} from "../../src/decorator/Get"; -import {createExpressServer, createKoaServer, getMetadataArgsStorage} from "../../src/index"; +import { createExpressServer, createKoaServer, getMetadataArgsStorage, NotAcceptableError } from "../../src/index"; import {assertRequest} from "./test-utils"; import {JsonController} from "../../src/decorator/JsonController"; import {Authorized} from "../../src/decorator/Authorized"; @@ -9,7 +9,7 @@ import {RoutingControllersOptions} from "../../src/RoutingControllersOptions"; const chakram = require("chakram"); const expect = chakram.expect; -describe("Controller responds with value when Authorization succeeds", function () { +describe("Controller responds with value when Authorization succeeds (async)", function () { before(() => { @@ -70,6 +70,67 @@ describe("Controller responds with value when Authorization succeeds", function }); +describe("Controller responds with value when Authorization succeeds (sync)", function () { + + before(() => { + + // reset metadata args storage + getMetadataArgsStorage().reset(); + + @JsonController() + class AuthController { + + @Authorized() + @Get("/auth1") + auth1() { + return { test: "auth1" }; + } + + @Authorized(["role1"]) + @Get("/auth2") + auth2() { + return { test: "auth2" }; + } + + } + }); + + const serverOptions: RoutingControllersOptions = { + authorizationChecker: (action: Action, roles?: string[]) => { + return true; + } + }; + + let expressApp: any; + before(done => { + const server = createExpressServer(serverOptions); + expressApp = server.listen(3001, done); + }); + after(done => expressApp.close(done)); + + let koaApp: any; + before(done => { + const server = createKoaServer(serverOptions); + koaApp = server.listen(3002, done); + }); + after(done => koaApp.close(done)); + + describe("without roles", () => { + assertRequest([3001, 3002], "get", "auth1", response => { + expect(response).to.have.status(200); + expect(response.body).to.eql({ test: "auth1" }); + }); + }); + + describe("with roles", () => { + assertRequest([3001, 3002], "get", "auth2", response => { + expect(response).to.have.status(200); + expect(response.body).to.eql({ test: "auth2" }); + }); + }); + +}); + describe("Authorized Decorators Http Status Code", function () { before(() => { @@ -129,48 +190,90 @@ describe("Authorized Decorators Http Status Code", function () { }); -describe("Authorization checker allows to throw", function() { - before(() => { - // reset metadata args storage - getMetadataArgsStorage().reset(); - - @JsonController() - class AuthController { - @Authorized() - @Get("/auth1") - auth1() { - return { test: "auth1" }; - } - - } - }); - - const serverOptions: RoutingControllersOptions = { - authorizationChecker: async (action: Action, roles?: string[]) => { - throw new Error('Custom Error'); - } - }; - - let expressApp: any; - before(done => { - const server = createExpressServer(serverOptions); - expressApp = server.listen(3001, done); - }); - after(done => expressApp.close(done)); - - let koaApp: any; - before(done => { - const server = createKoaServer(serverOptions); - koaApp = server.listen(3002, done); - }); - after(done => koaApp.close(done)); - - describe("custom errors", () => { - assertRequest([3001, 3002], "get", "auth1", response => { - expect(response).to.have.status(500); - expect(response.body).to.have.property("name", "Error"); - expect(response.body).to.have.property("message", "Custom Error"); - - }); - }); +describe("Authorization checker allows to throw (async)", function() { + before(() => { + // reset metadata args storage + getMetadataArgsStorage().reset(); + + @JsonController() + class AuthController { + @Authorized() + @Get("/auth1") + auth1() { + return { test: "auth1" }; + } + } + }); + + const serverOptions: RoutingControllersOptions = { + authorizationChecker: async (action: Action, roles?: string[]) => { + throw new NotAcceptableError("Custom Error"); + }, + }; + + let expressApp: any; + before(done => { + const server = createExpressServer(serverOptions); + expressApp = server.listen(3001, done); + }); + after(done => expressApp.close(done)); + + let koaApp: any; + before(done => { + const server = createKoaServer(serverOptions); + koaApp = server.listen(3002, done); + }); + after(done => koaApp.close(done)); + + describe("custom errors", () => { + assertRequest([3001, 3002], "get", "auth1", response => { + expect(response).to.have.status(406); + expect(response.body).to.have.property("name", "NotAcceptableError"); + expect(response.body).to.have.property("message", "Custom Error"); + }); + }); +}); + +describe("Authorization checker allows to throw (sync)", function() { + before(() => { + // reset metadata args storage + getMetadataArgsStorage().reset(); + + @JsonController() + class AuthController { + @Authorized() + @Get("/auth1") + auth1() { + return { test: "auth1" }; + } + } + }); + + const serverOptions: RoutingControllersOptions = { + authorizationChecker: (action: Action, roles?: string[]) => { + throw new NotAcceptableError("Custom Error"); + }, + }; + + let expressApp: any; + before(done => { + const server = createExpressServer(serverOptions); + expressApp = server.listen(3001, done); + }); + after(done => expressApp.close(done)); + + let koaApp: any; + before(done => { + const server = createKoaServer(serverOptions); + koaApp = server.listen(3002, done); + }); + after(done => koaApp.close(done)); + + describe("custom errors", () => { + assertRequest([3001, 3002], "get", "auth1", response => { + expect(response).to.have.status(406); + expect(response.body).to.have.property("name", "NotAcceptableError"); + expect(response.body).to.have.property("message", "Custom Error"); + }); + }); }); \ No newline at end of file