Skip to content
This repository has been archived by the owner on Jul 24, 2019. It is now read-only.

Decouples Express and adds request signing verification support #59

Merged
merged 4 commits into from
Aug 7, 2018

Conversation

shaydewael
Copy link
Contributor

Summary

Addresses #57 and decouples the adapter from express.

Creates a breaking change that adds support for signing secrets (https://api.slack.com/docs/verifying-requests-from-slack) over legacy verification tokens.

This also decouples the adapter from express, which includes adding parsing on the adapter level and refactoring express-middleware.js into http-handler.

Requirements (place an x in each [ ])

@codecov
Copy link

codecov bot commented Aug 6, 2018

Codecov Report

Merging #59 into master will increase coverage by 1.22%.
The diff coverage is 95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #59      +/-   ##
==========================================
+ Coverage   94.26%   95.48%   +1.22%     
==========================================
  Files           4        4              
  Lines         122      133      +11     
==========================================
+ Hits          115      127      +12     
+ Misses          7        6       -1
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️
src/adapter.js 97.82% <100%> (+0.32%) ⬆️
src/http-handler.js 93.75% <93.75%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 265f456...a599948. Read the comment docs.

@shaydewael shaydewael requested a review from aoberoi August 7, 2018 16:20
README.md Outdated
@@ -127,6 +125,8 @@ http.createServer(app).listen(port, () => {
});
```

> ⚠️ As of `v2.0.0`, the Events API adapter parses raw request bodies while performing request signing verification. This means developers no longer need to use `body-parser` middleware to parse urlencoded requests.
Copy link
Contributor

Choose a reason for hiding this comment

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

last few words -> parse JSON-encoded requests

@@ -1,6 +1,6 @@
{
"parser": "babel-eslint",
"extends": [ "airbnb" ],
"extends": [ "airbnb-base" ],
Copy link
Contributor

Choose a reason for hiding this comment

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

YAAAS

import { packageIdentifier } from './util';

export const errorCodes = {
NO_BODY_PARSER: 'SLACKEVENTMIDDLEWARE_NO_BODY_PARSER',
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this


export const errorCodes = {
NO_BODY_PARSER: 'SLACKEVENTMIDDLEWARE_NO_BODY_PARSER',
SIGNATURE_VERIFICATION_FAILURE: 'SLACKMESSAGEMIDDLEWARE_REQUEST_SIGNATURE_VERIFICATION_FAILURE',
Copy link
Contributor

Choose a reason for hiding this comment

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

SLACKMESSAGEMIDDLEWARE -> SLACKHTTPHANDLER for consistency

const debug = debugFactory('@slack/events-api:http-handler');

export function createHTTPHandler(adapter) {
// Removed middlewareOptions because no next() -- is this okay?
Copy link
Contributor

Choose a reason for hiding this comment

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

i think yes, because propogateErrors is no longer an option, so it doesn't have a purpose. can be introduced later in a backwards-compatible way.

try {
if (adapter.waitForResponse) {
adapter.emit('error', error, respond);
} else if (process.env.NODE_ENV === 'development') {
Copy link
Contributor

Choose a reason for hiding this comment

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

good call!

.end(function (err, res) {
assert(err instanceof Error);
assert.equal(res.statusCode, 500);
assert.equal(res.statusCode, 404);
Copy link
Contributor

Choose a reason for hiding this comment

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

good call again!

@aoberoi
Copy link
Contributor

aoberoi commented Aug 7, 2018

fixes #36

// *** Initialize event adapter using verification token from environment variables ***
const slackEvents = slackEventsApi.createSlackEventAdapter(process.env.SLACK_VERIFICATION_TOKEN, {
// *** Initialize event adapter using signing secret from environment variables ***
const slackEvents = slackEventsApi.createSlackEventAdapter(process.env.SIGNING_SECRET, {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should also update the package.json of this example to reference the yet-to-be-published v2.0.0 so that this actually runs.

package.json Outdated
@@ -33,7 +32,7 @@
"babel-plugin-system-import-transformer": "^2.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

should prob remove this and the devDep above it. we're no longer using system import or dynamic import anywhere, right?

@shaydewael shaydewael merged commit 8b950e0 into slackapi:master Aug 7, 2018
@shaydewael shaydewael deleted the express_decoupling branch August 7, 2018 23:14
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.

None yet

2 participants