routing-controllers icon indicating copy to clipboard operation
routing-controllers copied to clipboard

fix: Authorization checker is executer after middlewares

Open killix opened this issue 4 years ago • 3 comments

Description

The authorization checker is executed after the middlewares (beforeMiddlewares).

Minimal code-snippet showcasing the problem

import 'reflect-metadata';
import { /*createExpressServer,*/ createKoaServer, JsonController, Authorized, Get, Action, UseBefore } from 'routing-controllers';
import { KoaBeforMiddleware } from './KoaMiddleware';

function KoaBeforMiddleware(context: any, next: (err?: any) => Promise<any>): Promise<any> {
  console.log('do something before execution...');
  return next()
    .then(() => {
      console.log('do something after execution');
    })
    .catch(error => {
      console.log('error handling is also here');
    });
}

@JsonController()
export class APIController {

  @Authorized()
  @Get('/items')
  @UseBefore(KoaBeforMiddleware)
  async getItems() {
    return [];
  }
}

const authorizationChecker = async (action: Action, roles: string[]) => {
  console.log('authorizationChecker')
  const token = action.request.headers["authorization"];
  return token ? true : false;
};

// createExpressServer({ authorizationChecker }).listen(3000);
createKoaServer({ authorizationChecker }).listen(3001);

Expected behavior

authorizationChecker is called before the middleware

Actual behavior

authorizationChecker is called after the middleware

killix avatar Apr 05 '21 21:04 killix

Hopefully this gets merged soon. I just ran into this issue myself.

ToryMakesThings avatar Apr 13 '21 22:04 ToryMakesThings

@killix @attilaorosz Hello! What ended up happening with this issue? This seems like a good change, but I can see the legacy PR was closed without too much explanation? Was it breaking the other middlewares?

As an interim fix, I think I will replace authorizationChecker with a middleware of higher priority to my current middlewares. Could a good solution be to add the priority key that currently lives on @Middleware to this in some way?

Johoseph avatar Jan 19 '24 05:01 Johoseph

@Johoseph Seems like the author of the PR didn't finish the changes (missing tests). This could indeed be a trivial change and if someone could pick it up I'd happy to merge it. I'm personally against the auth checker concept in general, I think the base framework should not be responsible for these kind of checks, that's why middlewares are a thing in the first place. Your solution of creating a simple middleware for this with a higher priority is exactly what I would do myself.

attilaorosz avatar Jan 19 '24 18:01 attilaorosz