Add Listener options and filtering hooks

[michaelansel]

Enable Listener hooks that allow for complex match rules.

Example: only execute commands for users who have performed 2FA with hubot.

Listener.hooks.push (robot, listener, options, message, cb) ->
  if options.auth?.require_2fa
    if robot.auth.getUserAuthState(message.message.user)
      cb(null, true)
    else
      message.reply "This command requires two factor authentication. Please run `#{robot.name} auth me` to level up."
      cb(null, false)
  else
    cb(null, true)

[samlambert]

I like this.

What do you think @technicalpickles

[michaelansel]

Forgot to add async as a dependency for hubot

[michaelansel]

Any chance of getting this merged? Is there anything I can do that would help?

[technicalpickles]

This would break all existing scripts. Maybe check the type of these arguments, and assign options to callback if it's a function?

[technicalpickles]

Looks like that is actually being handled in the TextListener constructor.

[technicalpickles]

Is there anything I can do that would help?

Any relevant updates to docs/scripting.md would be super helpful, otherwise no one would be able to discover you can do this without diving into the code.

[technicalpickles]

Why parallel? Listeners are generally invoked in series, so it might not be obvious that hooks work different.

Also, shouldn't we be doing something with this err?

[technicalpickles]

Listener.hooks.push (robot, listener, options, message, cb) ->

All interactions with hubot are done through the robot object, so I think it would make sense for the interface to be like robot.hook (listener, options, msg, cb) -> instead.

[technicalpickles]

Is this comment stale? If you get an err, you it should have the stacktrace at this point.

[technicalpickles]

So, it turns out this is something I've wanted for awhile, but I was just calling it something different: middleware. I had envisioned something like, passing a script name to a respond block, and then tracking metrics around how often it's called, or logging when scripts respond. Stuff like that.

Middleware might have certain connotations though. Usually, you are either preventing the real action from happening, or preparing results for other hooks and/or the real action

I'm not sure if I like hook though. What's it a hook for? Everything can have hooks really. Maybe receiveHook, since it's hooks about whether the bot actually receives it for processing?

To recap, my current concerns are:

• the name of the thing
• the interface for adding hooks
• documentation on using this

[michaelansel]

Thanks for the feedback! I'll start addressing each concern right away.

[michaelansel]

Wow, its been 3 weeks. Sorry about that.

I did a lot of thinking about what it feels like we are trying to solve here and realized that:

• maybe there are multiple pieces to this solution
• maybe none of them actually address the problem I want to solve (abstracted authentication)

First, a big fancy diagram of the current flow and a potential new flow (new flow is just adding in some new steps): https://www.lucidchart.com/documents/view/b7eade23-4254-45af-b164-2ad54674e61b/0

Current flow explained as I understand it:

• a new message is received and a message object is created
• all listeners are tested in order to see if they match the message
• if no listeners match, do something with CatchAllMessage to terminate message processing
• when a listener matches, execute the listener's handler/callback
• if the handler sets message.done=true, terminate message processing

Now, in the new flow, I am proposing we just extend the existing flow with three new constructs:

1. Hooks at various points in the process. Hooks are executed in order of registration, but have no impact on the message processing flow. These are for things like logging, collecting metrics, or triggering other processes (e.g. deprecation warnings or long-running command notifications).
2. Generalized filters that are glued onto existing listeners. This is what I originally designed for this PR. A filter is a block of code that, given a listener and message, decides whether or not the handler should be executed. These are intended to be a "all must approve" model in order to actually process the message. There is an open question around what happens when filters fail (do we terminate message processing or continue to the next listener?)
3. Explicit results from listener handlers: either you processed the message or you skipped it. If the message was processed, we should stop processing further. I am not hard set on this change, but it seems less surprising than the current model in which you need to remember to set message.done=true

With all of that described out, I am realizing that it still doesn't solve my original problem of wanting to glue company-specific auth logic on top of existing scripts without having to fork every single hubot script. I think I might be able to implement it with some potentially crazy code inside a filter, but I'm not convinced I want to go there. Ideas welcome here...

[michaelansel]

Just discovered http://github.com/Hammertime38/hubot-proxy, which looks like it may address some of the hooks.

[michaelansel]

Based on the updates to #768, I'm going to work on rebuilding this in smaller pieces. I expect the end result to be a collection of individual pieces that eventually enable a variety of extensions like external authorization policy, rate limiting, and conditional audit logging.

[michaelansel]

Closing this in favor of #801 and #803, which accomplish the same goals.