Feature Request: Inject middlewares to controllers with decorators

[maxmalov]

Hi,

I was looking at the example how to inject my middleware into my controllers and a few concers came to my mind.

First one is that every controller in this approach become container aware and it is very easy to start using it as a service locator everywhere within controller class. This can lead to an antipattern being spread across the majority of controllers, so the developer should constantly keep in mind while developing controllers.

Second one is that middleware injection usually is very common operation. From my experience I can say that authorization middleware presents in the majority of routes. So this construction from the example becomes a boilerplate.

I really like how such things implemented in .Net MVC, ex Authorize, AllowAnonymous attributes:

[Authorize]
public class SomeController() {

    public ActionResult Action1 // this action will be protected

    [AllowAnonymous]
    public ActionResult Action2 // only this method is not effected...
}

And I'm wondering is this possible with inversify too, in a way like:

// somewhere in the composition phase

container.bind<express.RequestHandler>(Constants.AuthorizeMiddleware).toConstantValue(checkAuth);
container.bind<express.RequestHandler>(Constants.AllowAnonymousMiddleware).toConstantValue(allowAnonymous);

// later in a controller

@Controller("/")
@Middleware(Constants.AuthorizeMiddleware)
class MyController {
  // this will be protected
  @Post("/")
  public create() { ... }

  // but this will be accessible w/o authentication
  @Get("/")
  @Middleware(Constants.AllowAnonymousMiddleware)
  public get() { ... }
}

Pros I see in this approach:

• Middleware injection via @Method decorator can be removed, which plays well with single responsibility principle
• Controllers have no references to the container by default. I'm not saying this is a silver bullet against service locator antipattern, but it makes it harder to achieve.
• Developers can easily create their own decorators based on Middleware one.

Any ideas, concerns?

Thanks

[remojansen]

Hi, thanks for this feature request. I believe this has been discussed in the past but I can't remember it 😢 . Let see if other members of the team can remember it...

Can you guys please review @codyjs @lholznagel thanks 👍

[lholznagel]

I remember that we wanted to do something like this in citadeljs.

I think this would be a good idea. 👍

[remojansen]

@maxmalov would you like to contribute this feature? I also think that it is a good idea 👍

I'm only wondering one thing...

At the moment you can pass middleware as:

@Controller(path, [middleware, ...])
// or
@Method(method, path, [middleware, ...])
// or
@SHORTCUT(path, [middleware, ...])

More info here.

I was wondering if we need:

A

An extra @ Middleware decorator:

@Controller("/")
@Middleware(Constants.AuthorizeMiddleware)
class MyController {
// ...

B

Or if we should allow serviceIdentifiers to be passed to the existing decorators:

@Controller("/", Constants.AuthorizeMiddleware)
class MyController {
// ...

A service identifier can be a class constructor function, a string or a Symbol and the middleware is a function. This means that detecting if the argument passes is a serviceIdentifier or a middleware is not straight forward but it is not impossible.

I think option A will be easier to implement. Should we call the decorator @Middleware or @InjectMiddleware?

What do you guys think?

[maxmalov]

@remojansen I'd really like to, but I'm fairly new with inversify, so I will definitely need you help and review at the start.

I'm common with current approach of injecting middleware, but have a few concerns about it. Since right not it is possible to inject express request handlers only you cannot control how the middleware is being created. I've done some digging already and come up to the following motivation:

• I need to stub middleware during testing
• I need to inject entities to middleware. For instance, we have authorization middleware which will validate user's roles and a service responsible for authorization. So I want to inject a service to middleware somehow. And since I'm fairly new with inversify the only solution I see now is to use factories:

// middleware factory definition

export interface AuthMiddlewareFactory {
  (authService: AuthService): express.RequestHandler;
}

function makeAuthMiddleware(authService): express.RequestHandler {
  return (req, res, next) => {
    // ...
  };
}

// configuration

container.bind<AuthService>("AuthService").toSelf();
container.bind<AuthMiddlewareFactory>("AuthMiddlewareFactory").toConstantValue(makeAuthMiddleware);

container.bind<interfaces.Factory<express.RequestHandler>>("Factory<AuthMiddleware>").toFactory<express.RequestHandler>((context: interfaces.Context) => {
    return () => {
        let service = context.container.get<AuthService>("AuthService");
        let makeAuthMiddleware = context.container.get<AuthMiddlewareFactory>("AuthMiddlewareFactory");
        return makeAuthMiddleware(service);
    };
});

Well, this is a lot of code for a single middleware and a lot of manual dependency resolutions which complicates this solution. So this is the point I stuck at, but I guess there should be another way...

[remojansen]

I'm going to separate your comments in two features.

A) Being able to stub middleware during testing

If your controller looks as follows:

let TYPE = {
    AuthorizeMiddleware: Symbol("AuthorizeMiddleware"),
    AllowAnonymousMiddleware: Symbol("AllowAnonymousMiddleware")
};

@injectable()
@Controller("/")
@Middleware(TYPE.AuthorizeMiddleware)
class MyController {

  @Post("/")
  public create() {
      // ...
  }

  @Get("/")
  @Middleware(TYPE.AllowAnonymousMiddleware)
  public get() {
      // ...
  }
}

And your middleware doesn't have dependencies:

let AuthorizeMiddleware = (req, res) => {
    // ...
};

let AllowAnonymousMiddleware = (req, res) => {
    // ...
};

Declaring the bindings should be fine:

let container = new Container();

container.bind<AuthorizeMiddleware>(TYPE.AuthorizeMiddleware)
         .toConstantValue(AuthorizeMiddleware);

container.bind<AllowAnonymousMiddleware>(TYPE.AllowAnonymousMiddleware)
         .toConstantValue(AllowAnonymousMiddleware);
         
container.bind<Controller>(TYPE.Controller)
         .to(MyController)
         .whenTargetNamed("MyController");

At the moment it is not possible to stub the middleware because we hard code a reference to the middleware:

@Controller("/", AuthorizeMiddleware)

But the @Middleware decorator will allow use to hard code a reference to an identifier of the middleware (not the middleware itself):

@Controller("/")
@Middleware(TYPE.AuthorizeMiddleware)

This will allow you to stub your middleware when testing. All you would need to do is to use different bindings for TYPE.AuthorizeMiddleware and TYPE.AllowAnonymousMiddleware during testing:

container.bind<AuthorizeMiddleware>(TYPE.AuthorizeMiddleware)
         .toConstantValue(function mock(req, res) {
              // ...
         });

container.bind<AllowAnonymousMiddleware>(TYPE.AllowAnonymousMiddleware)
         .toConstantValue(function mock(req, res) {
              // ...
         });

B) Being able to inject entities to middleware

Let's imagine that you have the same controller:

let TYPE = {
    AuthorizeMiddleware: Symbol("AuthorizeMiddleware"),
    AllowAnonymousMiddleware: Symbol("AllowAnonymousMiddleware")
};

@injectable()
@Controller("/")
@Middleware(TYPE.AuthorizeMiddleware)
class MyController {

  @Post("/")
  public create() {
      // ...
  }

  @Get("/")
  @Middleware(TYPE.AllowAnonymousMiddleware)
  public get() {
      // ...
  }
}

But this time the middleware requires a service to be injected:

let authorizeMiddlewareFactory = (authService) =>{
    return function authorizeMiddleware(req, res) => {
         // Use authService here...
    }
};

let AllowAnonymousMiddleware = (req, res) => {
    // ...
};

The solution you suggested would work but requires a lot of code. I would try with toDynamicValue:

let container = new Container();

container.bind<AutService>(TYPE.AutService).to(AutService);

container.bind<AuthorizeMiddleware>(TYPE.AuthorizeMiddleware)
         .toDynamicValue((context: interfaces.Context) => {
             let authService = context.container.get(TYPE.AutService);
             return authorizeMiddlewareFactory(authService);
         });

container.bind<AllowAnonymousMiddleware>(TYPE.AllowAnonymousMiddleware)
         .toConstantValue(AllowAnonymousMiddleware);
         
container.bind<Controller>(TYPE.Controller)
         .to(MyController)
         .whenTargetNamed("MyController");

If your middleware has more than one dependency:

container.bind<AuthorizeMiddleware>(TYPE.AuthorizeMiddleware)
         .toDynamicValue((context: interfaces.Context) => {
             let authService = context.container.get(TYPE.AutService);
             let logginService = context.container.get(TYPE.LogginService);
             return authorizeMiddlewareFactory(authService, logginService);
         });

It will start becoming ugly. In general, if you see container.get multiple times in your code you should consider it a code smell.

You could create a small helper to solve this problem:

function bindMiddleware(container, middlewareId, middlewareFactory, dependencies) {
    container.bind<AuthorizeMiddleware>(TYPE.AuthorizeMiddleware)
                   .toDynamicValue((context: interfaces.Context) => {
                       let injections = dependencies.map((dependency) => {
                           context.container.get(dependency);
                       });
                       return middlewareFactory(...injections);
                  });
}

You could then use this helper as follows:

let authorizeMiddlewareFactory = (authService, logginService) =>{
    return function authorizeMiddleware(req, res) => {
         // Use authService & logginService here...
    }
};

bindMiddleware(
    container,
    TYPE.AuthorizeMiddleware,
    authorizeMiddlewareFactory,
    [TYPE.AutService, TYPE.LogginService]
);

I think this is something we could document in the recipes but not something that should be part of the framework. The main problem here is that we are dealing with functions not with classes and we can't use decorators on functions. I saw online that there are conversations about adding decorator support for functions but we need to wait until there is more clarity on that...

If you want this helper (or something similar) to become a npm module you can always release it yourself.

Summary

• A) We need to implement @Middleware
• B) Once A has been implemented you will be able to use the helper described above.

The @Middleware decorator needs to be implemented using additional metadata here. This is not really a beginner feature so if you want to implement it I can try to do some analysis and comment here how I would implement it. I fyou think that it is too complex we will implement it and you can always help us to test if it solves your issue. Testing is also a very valuable contribution 👍

[maxmalov]

Wow, @remojansen, thanks for detailed explanation 👍. I'll play around with it and see what I can do.

[maxmalov]

@remojansen I've tried different approaches you've described previously (about the way of injecting middlewares). So I put all my current findings and thoughts below:

A) Inject middleware via @Controller and @Method decorators

here is the diff inversify/inversify-express-utils@master...maxmalov:middleware-injection-compat

and how it'll look like

let plain = (req, res, next) => { ... };
let stringId = "foo";
let symbolId = Symbol("bar"); 

@Controller("/", plain, stringId, symbolId)
class Controller {
  @Get("/", plain, stringId, symbolId)
  public getTest(req, res) { ... }
}

This is the easiest way to implement and use as a developer later in the code, I guess. Besides it does not break compatibility with current version of express utils. The main pro of this approach is that developer can specify order of middleware in an intuitive way (I'll cover that later in second approach).

B) Inject middleware via new @Middleware decorator

Here comes the tricky part, because order of decorators is really matters, ex

@Controller("/")
@Middleware(Middleware1)
@Middleware(Middleware2)
class Controller {}

So @Middleware cannot easily share the same metadata object with @Controller decorator, since it will be applied first. We need to store this metadata separately, that's okay. But there is also ambiguity related to the order; in the example above, @Middleware(Middleware2) will be applied before @Middleware(Middleware1). So we have a reverse order of middleware list and should keep this in mind later. I guess this is obvious for experienced developers, but not so much for starters. Since the order of middleware in express apps does really matter, there should be obvious way for developer to specify this order. @Middleware decorator can take a list, and overwrite the list of middleware each time it was applied, but it will be the same as case A), then.

Here is the diff with kind of in progress state (only controller level middleware injection) inversify/inversify-express-utils@master...maxmalov:middleware-injection-decorator

What do you think about these (at this moment I don't cover injecting entities to middleware 👻)?

[remojansen]

Hi @maxmalov thanks a lot for your work 👍 it looks very good. I like more option one. I only spotted one important required change. The Middleware interface needs to use the ServiceIdentifier<T> interface from inversify then you can removed the MiddlewareIdentifier interface:

import { interfaces } express from "inversify";
 
export type Middleware = (interfaces.ServiceIdentifier<any> | express.RequestHandler);

[mitjarogl]

@maxmalov, @remojansen Hey, great work. Is it possible to add this same feature into inversify-restify-utils package? Thanks.

[remojansen]

Hi @mitjarogl at the moment I don't have time because I'm writing a book after working hours. If you want to send a PR I will be happy to merge it. In Express it was implemented by inversify/inversify-express-utils#35 you can use it as reference.

[CVANCGCG]

@remojansen , @maxmalov: sorry in advance if I am not supposed to comment on a closed issue. I am looking to accomplish what you outlined at the very beginning; namely creating the equivalent of the [Authorize] filter/middleware that allows it to receive parameters for the role or roles required to execute the action/operation? After reading this issue and the latest inversify express utils page, I decided to create an implementation of BaseMiddleware that I am passing through to the @httpget|Delete|Apply decorators. It's working but is clunky because I haven't been able to discover how to pass parameters from the decorator to the middleware to allow me to designate which role is required per operation/action. Here's what I was looking to accomplish by using the same middleware shown below as AuthorizationFilter.

@httpget('/', AuthorizationFilter('ReviewerRole')) // for methods that don't require Admin role to execute
@httpDelete('/', AuthorizationFilter('AdministratorRole')) // for method that requires Admin role

What have I done? I created this middleware but it's hard-coded to with the use of 'AdministratorRole'. This is a problem because what I'd like to do is to reuse decorator and middleware as shown below in the ReportController.

class AuthorizationFilter extends BaseMiddleware {
public handler(req: express.Request, res: express.Response, next: express.NextFunction) {
try {
Guards.authorizedWithRole('AdministratorRole', this.httpContext.user.details);
} catch (exception) {
throw new HTTP401Error(exception);
}
next();
}
}
This class is currently hardcoding a role of AdministratorRole which makes single purpose and not reusable. My hope was to pass in the required role as a parameter in the use of the middleware in my annotation like this.

@controller('')
export class ReportController extends BaseHttpController {
constructor(@Inject(ReportService) private readonly _reportsService: ReportService) {
super();
}

// Here I would like to pass in 'ReviewerRole' since this is just a read only operation but don't know how to pass this to the AuthorizationFilter (BaseMiddleware from above)
@httpget('/reports/:id', **AuthorizationFilter('ReviewerRole')*)
public async handleGetPortfolioOverviewByManager(
): Promise { ... }

// Here I would like to pass in AdministratorRole' since this is a delete but don't know how to pass this to the AuthorizationFilter (BaseMiddleware from above)
@httpDelete('/reports/:id', **AuthorizationFilter('AdministratorRole')*)
public async handleGetPortfolioOverviewByManager(
): Promise {

My intuition is that this is already possible. I'd appreciate your guidance on the correct way to accomplish this.

[maxmalov]

@CVANCGCG as far as I can remember this is not currently possible. @httpMethod decorators accept service identifiers (or middleware functions) only and resolve them to a kind of "hardcoded" middleware instances.

I would recommend trying a different approach based on auth provider and principals https://github.com/inversify/inversify-express-utils#authprovider

Or you can split authorization filter into 2 separate middlewares: the first one will inject the given role inside req (the old fashion way 😄) and the second one will pick the role from the req and filter requests. Thus we manually inject dependencies using req object.

// this is pretty simple middleware which doesn't need any dependency resolution
const RequredRole = role => (req, res, next) => {
  req.requiredRole = role; // or use Symbols instead of public property access
  next();
}

// controller class

@httpget('/reports/:id', RequredRole('ReviewerRole'), AuthorizeByRoleFilter)
public async handleGetPortfolioOverviewByManager(
): Promise { ... }

Also, request scope services might look very promising https://github.com/inversify/inversify-express-utils#request-scope-services

[CVANCGCG]

@maxmalov, thank you for the detailed recommendation and just as much, I appreciate the quick response.

Yes, in fact, I am using the AuthProvider based solution to gain the Principal instance that wraps the user identity object exposed via the details getter. What I've just discovered was how to the middleware as a ServiceIdentifier (string) to associate inject 1:N named instances of the AuthorizationFilter.

The 3-part solution looks like this:
Part 1: Controller and use of decorator changing from an instance of interfaces.Middleware type to ServiceIdentifer:
Before:
@controller('', AuthorizationFilter)
After:
@controller('', TYPE.ReviewerRole)
And in

Part 2: Service Identifier Definition and Container registration
Added ServiceIdentifier constant:

export const TYPE = {
    AdministratorRole: Symbol.for('AuthorizationFilter'),
    ReviewerRole: Symbol.for('AuthorizationFilter'),
};

Before:

DIContainer.bind<AuthorizationFilter>(AuthorizationFilter)
    .inRequestScope();

After:

DIContainer.bind<AuthorizationFilter>(TYPE.AdministratorRole)
    .toDynamicValue(() => {
        return new AuthorizationFilter("AdministratorRole");
    })
    .inRequestScope();

DIContainer.bind<AuthorizationFilter>(TYPE.ReviewerRole)
    .toDynamicValue(() => {
        return new AuthorizationFilter("AdministratorRole");
    })
    .inRequestScope();

Part 3: AuthorizationFilter BaseMiddleware state and constructor modification
Before: as shown in previous comment/post
After:

class AuthorizationFilter extends BaseMiddleware {
    // Added constructor and state
    constructor(private _role: string) {
        super();
    }
    
    public handler(req: express.Request, res: express.Response, next: express.NextFunction) { 
    // Now we can acccess **this._role** or any additional instance variables to authorize or 
     reject the request by throwing an exception that returns an HTTP 401/403 Exception 
}

And for actions that require more elevated permissions to authorize:

@httpDelete('/reports/:id', TYPE.AdministratorRole)
public async handleGetPortfolioOverviewByManager(
): Promise {

[maxmalov]

Yeah, your solution works too, it also can be modified to look like in your initial question. Factories can produce everything, even service identifiers 🙃

const AuthorizationFilter = role => `AuthorizationFilter_${role}`;

// configuration phase

for (const role of ROLES) {
  DIContainer.bind<AuthorizationFilter>(AuthorizationFilter(role))
    .toDynamicValue(() => {
        return new AuthorizationFilter(role);
    })
    .inRequestScope();
}

// usage

@httpDelete('/reports/:id', AuthorizationFilter('AdministratorRole'))
public async handleGetPortfolioOverviewByManager(
): Promise {

but keep in mind this solution works only when the set of roles is predefined and won't change in runtime

[CVANCGCG]

@maxmalov, that's great insight. Your solution is more extensible and in IMO, is more intuitive to understand -- especially for those who are not intimately familiar with Service Identifiers as another way to resolve dependencies in addition to an explicit Middleware type reference. I'll give this a try. Thanks for the additional thought provoking enhancement.

Aside: I am still not proud of the fact my solution introduces state into the AuthorizationFilter. This feels like a workaround but maybe it's OK since we're working with classes rather than just a series of chained functions.