middleware proposal

[JacobGadawski]

This is My proposition to add middlewares to app instance in BootBot class. I want to add Raven/Sentry to my application but there is no option to add middlewares.

[mraaroncruz]

This gives us an opportunity to make this optional (but defaulting to active) in case you are handling other kinds of non-bot http requests with this express instance.
I, for example, have had to patch this in my own multipage bot in the past.

So we could make _verifyRequestSignature "public", add a boolean this.shouldVerifyRequest or something, set that default to true and also add it before setBeforeMiddlewares so we can halt the request before hitting other middlewares so we don't waste cpu cycles.

Or do you have a reason for setting it after setBeforeMiddlewares @JacobGadawski?

[mraaroncruz]

I like it. Simple and to the point.
There has been a request for this before. It needs tests, but let's see what @Charca thinks before you add them.

[Charca]

These two methods are identical, you can just have a setMiddlewares() method and you just call it before and after initializing the webhooks with different params.

[Charca]

@JacobGadawski, this is looking good. Just a few things I'd like to see in this PR before merging:

1. Consolidate the two new methods into a single one.
2. Add tests (at least for the new method)
3. Update the README file with instructions on how to use these new options.

Let me know if you have any questions. Thanks!

[mraaroncruz]

I think we should add two methods:

• bot.addBotMiddleware
• bot.addExpressMiddleware (naming can change)

The bot needs the _verifyRequestSignature middleware but if someone wants to add non-bot-related post or get handlers to the bot.app, they shouldn't be forced into also requiring a _verifyRequestSignature compatible header.
It could be implemented by conditionally adding middleware if the request.path doesn't match bot.webhook

Here's an example: https://stackoverflow.com/a/21293587/376680

What do you think @Charca ?

[Charca]

I like the idea of making the _verifyRequestSignature middleware optional, but I think it should be controlled by a config in the constructor's options object like you suggested in your original comment (something like shouldVerifyRequest: true | false).

Also, what's the reason we want to allow setting middlewares both before and after initializing the webhook?

[mraaroncruz]

After a discussion with a bootbot user in Slack, I believe that we shouldn't allow _verfiyRequestSignature to be optional, but instead give the user a possibility to add bot or app specific middleware.

I cannot foresee a situation where you would not need _veryfiRequestSignature on the actual webhook. So I believe the most flexible, while still being secure, way to let people extend bot.app would be to:

• make _verifyRequestSignature public => users could add it to other endpoints if necessary?
• only bind the bot.webhook path to _verifyRequestSignature
• allow users to either add a middleware to bot.app as custom HTTP request handlers or add middleware to the webhook/bot with another method.

This is the gist of what I was trying to say above. With the naming being:

• bot.addBotMiddleware
• bot.addExpressMiddleware (naming can change)

---

As far as after middleware goes, I have no opinion. I feel like it is a common feature in middleware libraries, but that's as far as my opinion and expertise go.

[mraaroncruz]

Adding the middleware to only bot.webhook may break backwards compatibility for some people who have had to hack the current implementation. Just a heads up.