Add support for request signing

[shaydewael]

Description

Request signing went live and we should add support into our SDKs. https://api.slack.com/docs/verifying-requests-from-slack

What type of issue is this? (place an x in one of the [ ])

• bug
• enhancement (feature request)
• question
• documentation related
• testing related
• discussion

Requirements

• I've read and understood the Contributing guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.
• I've searched for any related issues and avoided creating a duplicate issue.

[aoberoi]

just wanted to drop a proposal for the concrete implementation changes we could do to add this feature, and to address #31.

1. Implement a requestListener() method on the SlackMessageAdapter class. This method would return a function that conforms to the Node's http.Server's request event interface. That is to say, its parameters are (req, res).

   The function should encapsulate all the HTTP specifics before passing a payload to the adapter's dispatch method, and also serialize the return value into an HTTP response. It should functionally cover everything the expressMiddleware() method's returned function does (including request verification). The implementation may or may not use the body-parser npm package, and if so, it would become a direct dependency rather than an optional dependency.

2. Refactor the createServer() method of SlackMessageAdapter to return http.createServer(this.requestListener()) (or equivalent). At this point, express should no longer appear in the optional dependencies.

3. Refactor the expressMiddleware() method of SlackMessageAdapter to return a thin wrapper around this.requestListener() to adapt it to the middleware function signature. An example:

   function expressMiddleware() {
     const requestListener = this.requestListener();
     return (req, res, next) => {
       try {
         requestListener(req, res);
       } catch (error) {
         next(error);
       }
     };
   }

   The functionality would be exactly the same, except that this method would never throw the error for missing body-parser, because its no longer required. The slightly changed method signature is accomplished by catching thrown errors to a dedicated error handler via the next() middleware chaining mechanism.

[aoberoi]

another question is how we'd change the constructor's signature in order to support request signing. i believe the currently favored approach looks like the following:

using request signing (preferred):

const slackInteractions = createMessageAdapter(signingSecret, { verificationMethod: 'requestSignature' });

using a verification token (legacy):

const slackInteractions = createMessageAdapter(verificationToken);

in the legacy version, leaving out the verificationMethod option means using the default value of 'token'. you could explicitly use that value and achieve the same result.

const slackInteractions = createMessageAdapter(verificationToken, { verificationMethod: 'token' });

---

i'd like to propose an alternative to the above.

using request signing (preferred):

const slackInteractions = createMessageAdapter({ signingSecret });

using a verification token (legacy):

const slackInteractions = createMessageAdapter(verificationToken);

in the legacy version, using a string as the first argument is equivalent to passing in an object with the key 'verificationToken' set to that string as the value. you could explicitly use an object of this shape to achieve the same result.

const slackInteractions = createMessageAdapter({ verificationToken });

i think the latter proposal is a little more concise and clear. the first argument always completely describes the verification technique, and its required. in fact, there's room (although i'm not sure theres a motivation) to support using both request signing and verification token but simply setting both keys.

[shaydewael]

@Roach and I discussed another possibility for allowing the signing secret: keep the first parameter as a string, then create an optional flag disallowVerificationToken (defaulted to false).

const slackInteractions = createMessageAdapter(signingSecret, { disallowVerificationToken: true });

In this case, we would first attempt to verify the signature with the passed string, assuming it's a signing secret. This step is always done. If the verification fails, then we check if the optional flag is set.

If disallowVerificationToken is set to false (or isn't set), we assume the string passed is a verification token. We then check if the verification token matches the request just like we do in today's world. This allows us to also send a warning to the developer about future deprecation of the verification token.

If disallowVerificationToken is set to true (which we'd encourage), the verification would just fail.

I'm okay with the other proposal you sent, but it seems weird for the default verification method to be in an object, and forces us to keep that constructor signature even after verification tokens are deprecated in the future.

[aoberoi]

as far as i can tell, that proposal is roughly equivalent to the first proposal except:

1. its using a boolean rather than a string enum to signal how to do verification in the options
2. the preferred technique uses the key disallowVerificationToken.

IMHO, both of these have negative consequences that make the solution worse. using a boolean means there's only two discrete values, true or false. that means there will never be a third path (until, of course, we ship a new major version where we can revisit this). using a negative phrase in the preferred technique also reads as if we're not doing verification. to a beginner, disallowVerificationToken may quickly come off as "don't do verification". the difference between a signing secret and verification token are likely to be subtle. of course, this last criticism can be rectified by rephrasing in as an affirmative such as verifyRequestSignature.

while i think the last proposal is still roughly equivalent to the first, i think my second proposal still has some strengths over both.

1. the preferred syntax is more concise. yes, wrapping in an object is awkward. but which is more awkward when compared to always passing in an options object? i think the latter.

2. a cleaner separation of concerns: the first argument encapsulates everything we know about verification and verification is required. it can be interpreted to have a meaning all on its own, so it can be directly (or after munging) stashed in an instance property: verificationConfig. the second argument isn't required to make sense of the first.

[aoberoi]

okay lets put some names on these proposals so we can talk about them.

proposal a: the one with createMessageAdapter(signingSecret, { verificationMethod: 'requestSignature' })

proposal b: the one with createMessageAdapter({ signingSecret });

proposal c: the one with createMessageAdapter(signingSecret, { disallowVerificationToken: true })

while we're enumerating ideas, here's another one to put on the table.

proposal YOLO: major version bump and only accept the signing secret! also gives us a chance to rename SlackMessageAdapter to something that encapsulates all interactivity, including actions. maybe SlackInteractionAdapter?

[shaydewael]

Addressed with v1.0.0 🎉