Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggestion: Exclude routes from MiddlewareBuilder #17

Closed
cdiaz opened this issue Apr 15, 2017 · 17 comments
Closed

Suggestion: Exclude routes from MiddlewareBuilder #17

cdiaz opened this issue Apr 15, 2017 · 17 comments

Comments

@cdiaz
Copy link

cdiaz commented Apr 15, 2017

In some cases, a middleware is applied to all routes of a controller, except some.
To avoid having to write all in forRoutes(), there may be a way to exclude: exceptRoutes().
It's just an idea

@thomrick
Copy link
Contributor

Hey @cdiaz,

The idea is very cool !
Do you have some use cases with 'express' without nest ?

@cdiaz
Copy link
Author

cdiaz commented Apr 18, 2017

Hi @thomrick, It's the basic way I do it with express:

let except = function(path, middleware) {
  return function(req, res, next) {
      if (path.indexOf(req.path) > -1) {
          // Exclude 
          return next()
      } 
      else {
          // Apply for all others
          return middleware(req, res, next)
      }
  }
}
  app.use(except(['/some', '/another'], authMiddleware()));

I think it can be implemented in Nest, something like this:

builder.apply(authMiddleware)
    .forRoutes(UsersController)
    .except({
        path: ['/some', '/another']
    });

@thomrick
Copy link
Contributor

Hey @cdiaz,

I looked up on the Nest documentation and I found this:

When you pass UsersController in forRoutes method, Nest will setup middleware for each route in controller:

GET: users
GET: users/:id 
POST: users

But it is also possible to directly define for which path middleware should be used, just like that:

builder.apply(AuthMiddleware)
        .forRoutes({ path: '*', method: RequestMethod.ALL });

It isn't be better than adding new methods ?
Can you try this in your use cases and see if it's cool enough ?

@cdiaz
Copy link
Author

cdiaz commented Apr 19, 2017

@thomrick Assuming my controller has 20 routes and I need to exclude 2, it is better to write only 2 to exclude them instead of write 18 explicitly

@thomrick
Copy link
Contributor

@cdiaz in fact you make a good point if you write these 20 routes in 1 Controller !!!

But why writing 20 routes in 1 Controller ?

This problem can't be solved be refining architecture / design ?
And so on applying Middlewares on concerned Controllers ?

@cdiaz
Copy link
Author

cdiaz commented Apr 19, 2017

I was referring to a hypothetical case, it all depends on the requirements.

in fact I was researching and I found this package.
This it has about 165,622 downloads on the last month, therefore it can be considered that the exclusion of routes in a middleware is a common need of hight demand for many people.

@kamilmysliwiec
Copy link
Member

Hi @cdiaz,

You can just use with() to pass custom arguments to resolve() method.

resolve(path) {
    return (req, res, next) => {
        if (path.indexOf(req.path) > -1) {
            next();
        }
        ...
    };
}

@Zeldaze
Copy link

Zeldaze commented Nov 28, 2017

For those interested, I needed similar functionality in some of my middleware, particularly my AuthModule where I needed to allow access to one route, but not the rest in a particular controller. I followed this, and wanted to come up with a more standardised way of excluding the route.

    consumer
      .apply(AuthMiddleware)
      .with(
        { path: '/public/content/within/private/controller', method: RequestMethod.GET }
      )
      .forRoutes(PrviateContentController)

And my middleware as follows

interface Route {
    path: string;
    method: string;
}

@Middleware()
export class AuthMiddleware implements NestMiddleware {
  async resolve(...excludedRoutes: Route[]): Promise<ExpressMiddleware> {
    return async (req, res, next) => {
        if(
            excludedRoutes.filter(excludedRoute => {
                return excludedRoute.path == req.path && (excludedRoute.method === RequestMethod[req.method] || req.method === RequestMethod.ALL);
            }).length
        ){
            next();
        }else{
            runMyAuthMethod(req, res, next);
        }

    };
 }
}

Let me know what you think/any improvements as I am still learning.

@wbhob
Copy link
Contributor

wbhob commented Nov 28, 2017

maybe forRoutes return the method exclude, so you can exclude right after defining routes for which to apply the middleware.

    consumer
      .apply(AuthMiddleware)
      .forRoutes({ path: '*', method: RequestMethod.ALL })
      .exclude({ path: '/auth', method: RequestMethod.ALL }) //or `excludeRoutes`

@Zeldaze
Copy link

Zeldaze commented Nov 28, 2017

I agree something like that would be ideal, but @kamilmysliwiec already proposed solution.. I guess it's not a major issue at the moment

@wbhob
Copy link
Contributor

wbhob commented Nov 28, 2017

Why not have both 🎉

@BorntraegerMarc
Copy link

Definitely the suggestion of @wbhob is needed @kamilmysliwiec

@weeco
Copy link

weeco commented Apr 30, 2018

@kamilmysliwiec Can you reopen this issue or do you reject the feature as suggested by @wbhob ? I agree it would be very handy.

My example use case:
I have a global JWT authentication which is supposed to be disabled for the status route (where you can check the API's status) though.

@ayush987goyal
Copy link

Reiterating the previous comment, can this issue be reopened?
It is a typical case of excluding some route/methods of a Controller from the middleware and there is no easy way around it.

@wbhob
Copy link
Contributor

wbhob commented Jun 18, 2018

Recreated in #790. Please continue all conversation there, and upvote if you need this functionality.

@webhacking
Copy link

Is it still possible exclude methods?

@lock
Copy link

lock bot commented Sep 24, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants