Skip to content
This repository has been archived by the owner on Sep 20, 2024. It is now read-only.

Adds authentication of incoming Slack requests #167

Closed
wants to merge 2 commits into from
Closed

Adds authentication of incoming Slack requests #167

wants to merge 2 commits into from

Conversation

sgud
Copy link

@sgud sgud commented Mar 31, 2016

Adds optional support for authenticating requests from
Slack slash commands. Validates that the request body
includes a known token.

@sgud sgud changed the title Adds Express middleware to authenticate incoming Slack requests Adds authentication of incoming Slack requests Mar 31, 2016
@benbrown
Copy link
Contributor

Good idea! Thanks for the PR.

Should this be on by default perhaps? What would that involve?

@sgud
Copy link
Author

sgud commented Mar 31, 2016

This can certainly be activated by default. I think it makes sense to activate it by default as you can ensure that all endpoints are only ever reached if the token is valid, thus securing the server more effectively. This is in contrast to the current security mechanism which involves validating the api token in the slash command event itself (i.e. the endpoint is hit and validation is taking place subsequently). However, the downside is that making this a default option could impact backwards compatibility (may need to discuss further).

In order to activate by default, the secureWebhookEndpoints() function needs only to be added as the first line of createWebhookEndpoints() and the parameters of createWebhookEndpoints() modified to include authentication tokens.

@sgud
Copy link
Author

sgud commented Mar 31, 2016

I've modified the PR such that webhook authentication is now a default feature when creating webhook endpoints. Furthermore, by leaving the secureWebhookEndpoints() function as optional, this shouldn't impact backwards compatibility.

*/

// the list of authorized tokens that can invoke slash comamnds
const TOKEN_NOT_FOUND = -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you replace const with var please? We're trying to support NodeJS 0.12.x also.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I've replaced const with var and fixed the comment in line 7.

@anonrig
Copy link
Contributor

anonrig commented Mar 31, 2016

What a nice PR! Congrats.

@sgud
Copy link
Author

sgud commented Apr 4, 2016

@benbrown @anonrig:
Just wanted to check and see if there are any other changes you need made to the PR before it can be merged in. Please let me know if anything comes up.

@steve-jansen
Copy link

FYI if you want to test drive this PR.

I did an npm pack build locally of this PR after rebasing from upstream/master (commit 940c3e0).

Usage:

npm install https://github.com/steve-jansen/botkit/releases/download/v0.0.15-request-token-auth/botkit-0.0.15.tgz

@benbrown
Copy link
Contributor

benbrown commented Apr 5, 2016

I'll merge this in ASAP!
Sorry for the delay - just had a kid!

@steve-jansen
Copy link

@benbrown congrats! I'm a proud dad of 3 these days...

The days are long, the years are short. Enjoy them!

@sgud
Copy link
Author

sgud commented Apr 5, 2016

@benbrown Congratulations on the baby, wish you the best!

@benbrown
Copy link
Contributor

This is fabulous! I couldn't do a direct merge of this PR, but all of this code and these new features are now included!

@benbrown benbrown closed this May 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants