Merge pull request #178 from Diluka/fix/auth-decorator-status-code

Fix/auth decorator status code

Author: pleerock

src/driver/express/ExpressDriver.ts

@@ -12,6 +12,7 @@ import {AccessDeniedError} from "../../error/AccessDeniedError";
 import {AuthorizationCheckerNotDefinedError} from "../../error/AuthorizationCheckerNotDefinedError";
 import {isPromiseLike} from "../../util/isPromiseLike";
 import {getFromContainer} from "../../container";
+import {AuthorizationRequiredError} from "../../error/AuthorizationRequiredError";
 const cookie = require("cookie");
 const templateUrl = require("template-url");
 
@@ -114,21 +115,20 @@ export class ExpressDriver extends BaseDriver implements Driver {
 
                 const action: Action = {request, response, next};
                 const checkResult = this.authorizationChecker(action, actionMetadata.authorizedRoles);
-                if (isPromiseLike(checkResult)) {
-                    checkResult.then(result => {
-                        if (!result) {
-                            return this.handleError(new AccessDeniedError(action), actionMetadata, action);
 
-                        } else {
-                            next();
-                        }
-                    });
-                } else {
-                    if (!checkResult) {
-                        this.handleError(new AccessDeniedError(action), actionMetadata, action);
+                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 {
                         next();
                     }
+                };
+
+                if (isPromiseLike(checkResult)) {
+                    checkResult.then(result => handleError(result));
+                } else {
+                    handleError(checkResult);
                 }
             });
         }

src/driver/koa/KoaDriver.ts

@@ -12,6 +12,7 @@ import {AccessDeniedError} from "../../error/AccessDeniedError";
 import {isPromiseLike} from "../../util/isPromiseLike";
 import {getFromContainer} from "../../container";
 import {RoleChecker} from "../../RoleChecker";
+import {AuthorizationRequiredError} from "../../error/AuthorizationRequiredError";
 const cookie = require("cookie");
 const templateUrl = require("template-url");
 
@@ -92,21 +93,19 @@ export class KoaDriver extends BaseDriver implements Driver {
                     getFromContainer<RoleChecker>(actionMetadata.authorizedRoles).check(action) :
                     this.authorizationChecker(action, actionMetadata.authorizedRoles);
 
-                if (isPromiseLike(checkResult)) {
-                    return checkResult.then(result => {
-                        if (!result) {
-                            return this.handleError(new AccessDeniedError(action), actionMetadata, action);
-
-                        } else {
-                            return next();
-                        }
-                    });
-                } else {
-                    if (!checkResult) {
-                        return this.handleError(new AccessDeniedError(action), actionMetadata, action);
+                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();
+                        next();
                     }
+                };
+
+                if (isPromiseLike(checkResult)) {
+                    checkResult.then(result => handleError(result));
+                } else {
+                    handleError(checkResult);
                 }
             });
         }

src/error/AccessDeniedError.ts

@@ -1,10 +1,10 @@
 import {Action} from "../Action";
-import {UnauthorizedError} from "../http-error/UnauthorizedError";
+import {ForbiddenError} from "../http-error/ForbiddenError";
 
 /**
  * Thrown when route is guarded by @Authorized decorator.
  */
-export class AccessDeniedError extends UnauthorizedError {
+export class AccessDeniedError extends ForbiddenError {
 
     name = "AccessDeniedError";
 

src/metadata/ActionMetadata.ts

@@ -56,12 +56,12 @@ export class ActionMetadata {
     /**
      * Route to be registered for the action.
      */
-    route: string|RegExp;
+    route: string | RegExp;
 
     /**
      * Full route to this action (includes controller base route).
      */
-    fullRoute: string|RegExp;
+    fullRoute: string | RegExp;
 
     /**
      * Indicates if this action uses Body.
@@ -96,12 +96,12 @@ export class ActionMetadata {
     /**
      * Http code to be used on undefined action returned content.
      */
-    undefinedResultCode: number|Function;
+    undefinedResultCode: number | Function;
 
     /**
      * Http code to be used on null action returned content.
      */
-    nullResultCode: number|Function;
+    nullResultCode: number | Function;
 
     /**
      * Http code to be set on successful response.
@@ -141,12 +141,12 @@ export class ActionMetadata {
     /**
      * Special function that will be called instead of orignal method of the target.
      */
-    methodOverride?: (actionMetadata: ActionMetadata, action: Action, params: any[]) => Promise<any>|any;
+    methodOverride?: (actionMetadata: ActionMetadata, action: Action, params: any[]) => Promise<any> | any;
 
     // -------------------------------------------------------------------------
     // Constructor
     // -------------------------------------------------------------------------
-    
+
     constructor(controllerMetadata: ControllerMetadata, args: ActionMetadataArgs) {
         this.controllerMetadata = controllerMetadata;
         this.route = args.route;
@@ -197,7 +197,7 @@ export class ActionMetadata {
         this.headers = this.buildHeaders(responseHandlers);
 
         this.isAuthorizedUsed = this.controllerMetadata.isAuthorizedUsed || !!authorizedHandler;
-        this.authorizedRoles = (this.controllerMetadata.authorizedRoles || []).concat(authorizedHandler ? authorizedHandler.value : []);
+        this.authorizedRoles = (this.controllerMetadata.authorizedRoles || []).concat(authorizedHandler && authorizedHandler.value ? authorizedHandler.value : []);
     }
 
     // -------------------------------------------------------------------------
@@ -207,7 +207,7 @@ export class ActionMetadata {
     /**
      * Builds full action route.
      */
-    private buildFullRoute(): string|RegExp {
+    private buildFullRoute(): string | RegExp {
         if (this.route instanceof RegExp) {
             if (this.controllerMetadata.route) {
                 return ActionMetadata.appendBaseRoute(this.controllerMetadata.route, this.route);
@@ -263,7 +263,6 @@ export class ActionMetadata {
      */
     static appendBaseRoute(baseRoute: string, route: RegExp|string) {
         const prefix = `${baseRoute.length > 0 && baseRoute.indexOf("/") < 0 ? "/" : ""}${baseRoute}`;
-
         if (typeof route === "string")
             return `${prefix}${route}`;
 
@@ -273,5 +272,5 @@ export class ActionMetadata {
 
         return new RegExp(fullPath, route.flags);
     }
-    
+
 }

test/functional/auth-decorator.spec.ts

@@ -0,0 +1,69 @@
+import "reflect-metadata";
+import {Get} from "../../src/decorator/Get";
+import {createExpressServer, createKoaServer, getMetadataArgsStorage} from "../../src/index";
+import {assertRequest} from "./test-utils";
+import {JsonController} from "../../src/decorator/JsonController";
+import {Authorized} from "../../src/decorator/Authorized";
+import {Action} from "../../src/Action";
+import {RoutingControllersOptions} from "../../src/RoutingControllersOptions";
+const chakram = require("chakram");
+const expect = chakram.expect;
+
+describe("Authorized Decorators Http Status Code", 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: async (action: Action, roles?: string[]) => {
+            return false;
+        }
+    };
+
+    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(401);
+        });
+    });
+
+    describe("with roles", () => {
+        assertRequest([3001, 3002], "get", "auth2", response => {
+            expect(response).to.have.status(403);
+        });
+    });
+
+});