typestack / routing-controllers

Create structured, declarative and beautifully organized class-based controllers with heavy decorators usage in Express / Koa using TypeScript and Routing Controllers Framework.
MIT License
4.41k stars 394 forks source link

fix: Authorization checker is executer after middlewares #697

Open killix opened 3 years ago

killix commented 3 years ago

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

AgentGoldPaw commented 3 years ago

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

Johoseph commented 10 months ago

@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?

attilaorosz commented 10 months ago

@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.