Webhooks support for subscriber events #9230

kevinansfield · 2017-11-09T18:21:02Z

no issue

Support for http://resthooks.org style webhooks that can be used with Zapier triggers. This can currently be used in two ways:

a) adding a webhook record to the DB manually
b) using the API with password auth and POSTing to /webhooks/ (this is private API so not documented)

⚠️ only https URLs are supported in the webhook target_url field 🚨

add webhooks table to store event names and target urls
add POST and DELETE endpoints for /webhooks/
configure subscribers.added and subscribers.deleted events to trigger registered webhooks

kirrg001 · 2017-11-09T18:38:37Z

Note: We should merge and release #9199 before this. #9199 for Tuesday then 👍

jloh · 2017-11-09T21:14:21Z

Is the plan for these to be used for things other than zapier? ie I can create hooks to a gateway I've written to get events from ghost?

If so, are they going to be signed? It'd be great if they were signed like GitHub ones! https://developer.github.com/webhooks/securing/

kevinansfield · 2017-11-09T21:17:01Z

@jloh this is for outgoing webhooks, I don't think the GitHub style signed requests make sense here - you already need to have a client_id/secret and access token because the webhook endpoints are behind the authentication. It's a basic implementation of http://resthooks.org.

Right now we're only looking to do the basics that are needed to work with Zapier and will only work with a couple of events to start: subscriber.create and subscriber.delete

"New post" events in Zapier will be triggered via polling instead.

jloh · 2017-11-09T21:20:56Z

The receiver (zapier, other service etc) still need a way to verify that the webhook they receive is genuine and not from someone else. The GitHub method of signing hooks is mentioned on their security page http://resthooks.org/docs/security/

kevinansfield · 2017-11-09T21:25:56Z

Signing may well be added later on, it's not something we're looking at for this early version. Security with Zapier is handled through the unique target_url and UUID of the webhook in Ghost - to exploit the receiver in Zapier someone would have had to access your database or sniff HTTP traffic on the original webhook creation request if you are hosting your blog on HTTP instead of HTTPS.

core/server/api/index.js

+        } else if (result.hasOwnProperty('webhooks')) {
+            newObject = result.webhooks[0];
+            // TODO: this URL doesn't actually exist but is needed for a 201 response?
+            location = utils.url.urlJoin(apiRoot, 'webhooks', newObject.id, '/');


core/server/models/webhook.js

+        options = this.filterOptions(options, 'findAll');
+
+        return Webhook
+            .query({where: {event: event}})


core/server/webhooks-events.js

+    // webhook POST body should match GET request for same resource
+    // TODO: is `options` needed here?
+    var payload = {
+        subscribers: [subscriber.toJSON()]


core/server/webhooks-events.js

+}
+
+function triggerSubscriberRemoved(subscriber) {
+    // TODO: this will be difficult to make generic, maybe we need to listen


core/server/webhooks-events.js

@@ -0,0 +1,79 @@
+var _ = require('lodash'),


core/server/models/webhook.js

+        options = this.filterOptions(options, 'findAll');
+
+        return webhooksCollection
+            .query({where: {event: event}})


ErisDS

This is a very different pattern to any other code on the server side, so I can see that it's been a real struggle - this falls strongly in line with the conversation we had recently about having to invent everything all the time.

As the code works - it can be merged, no changes are really needed - however all of my comments are things we somehow need to codify into the codebase & surrounding documentation so that the way to solve these problems becomes obvious and not constant reinvention tests!

core/server/api/routes.js

@@ -201,5 +201,9 @@ module.exports = function apiRoutes() {
        api.http(api.redirects.upload)
    );

+    // ## Webhooks (RESTHooks)
+    apiRouter.post('/webhooks', mw.authenticatePrivate, api.http(api.webhooks.add));
+    apiRouter.delete('/webhooks/:id', mw.authenticatePrivate, api.http(api.webhooks.destroy));


core/server/api/webhooks.js

+                    }
+
+                    return models.Webhook.add(options.data.webhooks[0], _.omit(options, ['data'])).catch(function (error) {
+                        return Promise.reject(error);


core/server/models/webhook.js

+        options = this.filterOptions(options, 'findAll');
+
+        return webhooksCollection
+            .query({where: {event: event}})


core/server/webhooks-events.js

@@ -0,0 +1,79 @@
+var _ = require('lodash'),


core/server/webhooks-events.js

+    events = require('./events'),
+    logging = require('./logging'),
+    models = require('./models'),
+    https = require('https'),


core/server/webhooks-events.js

+}
+
+function triggerSubscriberRemoved(subscriber) {
+    // TODO: this will be difficult to make generic, maybe we need to listen


core/server/webhooks-events.js

+    };
+
+    // find relevant Webhooks
+    models.Webhook.findAllByEvent('subscriber.added').then(function (result) {


kevinansfield · 2017-11-17T00:17:35Z

@ErisDS I've attempted a refactor to make this more Ghost-like in 1e75a11, feel free to shoot that approach down if I've gone the wrong way, I'm certainly not attached to it! 😄

core/server/api/webhooks.js

+
+        tasks = [
+            doQuery,
+            _.partialRight(makeRequests, payload)


core/server/api/webhooks.js

+         * @returns {Object} options
+         */
+        function doQuery(options) {
+            return models.Webhook.getByEventAndTarget(options.data.webhooks[0].event, options.data.webhooks[0].target_url, _.omit(options, ['data']))


kevinansfield · 2017-11-21T11:21:32Z

Is that their official limit?

No, they don't have any official limits so it's possible they will use a different format in the future. This is an example of one of the resthook URLs they're currently creating:

https://hooks.zapier.com/hooks/standard/2665800/ae8965c1740e4db78571dfb486f7ccd0/
(81 chars)

If we were to limit the composite index to 191 chars it needs to be split between the event name and the URL, so something like 30/161. That's on the assumption we never have our own event names > 30 chars (maybe 35 would be more reasonable?) and resthook URLs of other services don't reach double the length of Zapier's which I think is reasonable but we have no data to back that up.

kevinansfield · 2017-11-21T12:15:13Z

I've been looking at other webhook URL examples

https://discordapp.com/api/webhooks/380692408364433418/mGLHSRyEoUaTvY91Te16WOT8Obn-BrJoiTNoxeUqhb6klKERb9xaZkUBYC5AfduwYCCy
(123 chars)

https://hooks.slack.com/services/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
(77 chars)

Discord is getting pretty close to the suggested 161 or 156 limit for target_url. For future compatibility I'd probably swing towards dropping the db constraint and reverting to the in-app validation when adding so there's no real chance of running into length limitations.

kevinansfield · 2017-11-21T12:41:02Z

I've reverted the db-level uniqueness constraint. Running a quick live test then I'll remove the WIP flag.

core/server/api/webhooks.js

+    docName = 'webhooks',
+    webhooks;
+
+// TODO: Use the request util. Do we want retries here?


core/server/api/webhooks.js

+        // when a webhook responds with a 410 Gone response we should remove the hook
+        if (err.status === 410) {
+            logging.info('webhook.destroy (410 response)', event, targetUrl);
+            return models.Webhook.destroy({id: webhookId});


core/server/data/migrations/versions/1.18/1-add-webhooks-table.js

+    return transacting.schema.hasTable(table)
+        .then(function (exists) {
+            if (exists) {
+                logging.warn(message);


core/server/models/webhook.js

+
+module.exports = {
+    Webhook: ghostBookshelf.model('Webhook', Webhook),
+    Webhooks: ghostBookshelf.collection('Webhook', Webhooks)


kevinansfield · 2017-11-21T14:35:57Z

@kirrg001 comments addressed

core/server/services/webhooks.js

+// register listeners only for events that have webhooks
+function listen() {
+    events.on('subscriber.added', _.partial(listener, 'subscriber.added'));
+    events.on('subscriber.deleted', _.partial(listener, 'subscriber.deleted'));


core/server/models/subscriber.js

@@ -19,16 +21,16 @@ Subscriber = ghostBookshelf.Model.extend({
        };
    },

-    onCreated: function onCreated(model) {
-        model.emitChange('added');
+    onCreated: function onCreated(model, options) {


core/server/api/webhooks.js

+// also known as "REST Hooks", see http://resthooks.org
+var Promise = require('bluebird'),
+    _ = require('lodash'),
+    https = require('https'),


kirrg001

Does not break anything. Should be good to go now.

refs TryGhost#9230

refs #9230

kevinansfield force-pushed the resthooks branch 3 times, most recently from 48da9dc to cd51797 Compare November 16, 2017 17:24

kevinansfield commented Nov 16, 2017

View reviewed changes

core/server/models/webhook.js Outdated

options = this.filterOptions(options, 'findAll');

return webhooksCollection

.query({where: {event: event}})

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

ErisDS reviewed Nov 16, 2017

View reviewed changes

kevinansfield commented Nov 17, 2017

View reviewed changes

core/server/api/webhooks.js Outdated

tasks = [

doQuery,

_.partialRight(makeRequests, payload)

This comment was marked as abuse.

Sign in to view

kirrg001 reviewed Nov 18, 2017

View reviewed changes

kevinansfield added 16 commits November 20, 2017 19:36

db migration for webhooks table

d08d5e0

add create/delete webhooks API routes

0625a40

bump permissions-roles call counts in tests

a94ba18

I should learn to test locally first 🙈

d47ddb1

bump hashes after rebase

32809dd

emit webhook model events - useful for adding/removing listeners later

bc46330

WIP: trigger webhooks on subscriber.added/deleted

b3621be

add Webhook.findAllByEvent (fixes webhook triggering for all rows)

e6a87df

handle 410 Gone response in webhook trigger, add logging

6080647

add missing options passthrough

0d0c9cb

fix webhooks findAllByEvent query

9013d70

minor tweaks

794e1d1

refactor to service, don't use models outside of API layer

2c4fbf0

add webhooks service tests, fix subscriber.deleted payload

c99bcf4

oops, node 4 <> es6

d8ec0cb

lowercase validation for webhook.event + webhook api integration tests

918533b

kevinansfield force-pushed the resthooks branch from 1e77fd3 to 918533b Compare November 21, 2017 12:37

log payload when webhook request fails

9d9222b

kirrg001 reviewed Nov 21, 2017

View reviewed changes

core/server/api/webhooks.js

docName = 'webhooks',

webhooks;

// TODO: Use the request util. Do we want retries here?

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

remove requirement for pluralize dependency

fe770ba

kevinansfield changed the title ~~[WIP] Webhooks/REST Hooks support~~ ✨ Support for subscribers Zapier integration Nov 21, 2017

kevinansfield changed the title ~~✨ Support for subscribers Zapier integration~~ ✨ Webhooks support for subscriber events Nov 21, 2017

kirrg001 reviewed Nov 21, 2017

View reviewed changes

core/server/data/migrations/versions/1.18/1-add-webhooks-table.js

return transacting.schema.hasTable(table)

.then(function (exists) {

if (exists) {

logging.warn(message);

This comment was marked as abuse.

Sign in to view

kirrg001 reviewed Nov 21, 2017

View reviewed changes

core/server/models/webhook.js Outdated

module.exports = {

Webhook: ghostBookshelf.model('Webhook', Webhook),

Webhooks: ghostBookshelf.collection('Webhook', Webhooks)

This comment was marked as abuse.

Sign in to view

kevinansfield added 4 commits November 21, 2017 14:26

pass options through to 410 Gone handling

951aee7

return early if table exists in migration

feecbf0

fix pluralisation in webhooks collection

3f48e4a

cleanup TODO comments

c17b00b

kirrg001 reviewed Nov 21, 2017

View reviewed changes

pass options through subscriber model events

6124b55

kirrg001 reviewed Nov 21, 2017

View reviewed changes

core/server/models/subscriber.js Outdated

@@ -19,16 +21,16 @@ Subscriber = ghostBookshelf.Model.extend({

};

},

onCreated: function onCreated(model) {

model.emitChange('added');

onCreated: function onCreated(model, options) {

This comment was marked as abuse.

Sign in to view

kevinansfield added 2 commits November 21, 2017 14:58

fix subscriber event arguments

473b9c5

pass options through webhook model events (match other models)

4f87f11

kirrg001 reviewed Nov 21, 2017

View reviewed changes

kevinansfield changed the title ~~✨ Webhooks support for subscriber events~~ Webhooks support for subscriber events Nov 21, 2017

kirrg001 approved these changes Nov 21, 2017

View reviewed changes

kevinansfield merged commit bffb3db into TryGhost:master Nov 21, 2017

kevinansfield deleted the resthooks branch November 21, 2017 15:43

kirrg001 added a commit to kirrg001/Ghost that referenced this pull request Dec 12, 2017

Migration for webhook permissions

d2f1ccf

refs TryGhost#9230

kirrg001 mentioned this pull request Dec 12, 2017

Migration for webhook permissions #9320

Merged

kevinansfield pushed a commit that referenced this pull request Dec 12, 2017

Migration for webhook permissions (#9320)

a98346e

refs #9230

Webhooks support for subscriber events #9230

Webhooks support for subscriber events #9230

Conversation

kevinansfield commented Nov 9, 2017 • edited Loading

kirrg001 commented Nov 9, 2017

jloh commented Nov 9, 2017

kevinansfield commented Nov 9, 2017 • edited Loading

jloh commented Nov 9, 2017

kevinansfield commented Nov 9, 2017 • edited Loading

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

ErisDS left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

kevinansfield commented Nov 17, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

kevinansfield commented Nov 21, 2017

kevinansfield commented Nov 21, 2017

kevinansfield commented Nov 21, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

kevinansfield commented Nov 21, 2017

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

kirrg001 left a comment

Choose a reason for hiding this comment

kevinansfield commented Nov 9, 2017 •

edited

Loading

kevinansfield commented Nov 9, 2017 •

edited

Loading

kevinansfield commented Nov 9, 2017 •

edited

Loading