From 52948a41654df69d67140c46dd8cac80772fdaa6 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 3 Apr 2017 16:51:14 -0600 Subject: [PATCH 01/12] Added support for a plugin which disables lazy assets --- client/coral-embed/src/index.js | 70 ++++++++----------- .../graphql/queries/streamQuery.graphql | 2 +- graph/resolvers/root_query.js | 5 -- routes/assets/index.js | 1 + routes/index.js | 1 + views/article.ejs | 3 +- views/embed-stream.ejs | 48 ------------- 7 files changed, 35 insertions(+), 95 deletions(-) delete mode 100644 views/embed-stream.ejs diff --git a/client/coral-embed/src/index.js b/client/coral-embed/src/index.js index e3350e0d3..9b1b29879 100644 --- a/client/coral-embed/src/index.js +++ b/client/coral-embed/src/index.js @@ -1,5 +1,7 @@ import pym from 'pym.js'; +import {stringify} from 'querystring'; + const snackbarStyles = { position: 'fixed', cursor: 'default', @@ -26,30 +28,21 @@ const Coral = {}; const Talk = Coral.Talk = {}; // build the URL to load in the pym iframe -function buildStreamIframeUrl(talkBaseUrl, asset_url, comment, asset_id) { - let iframeArray = [ +function buildStreamIframeUrl(talkBaseUrl, query) { + let url = [ talkBaseUrl, (talkBaseUrl.match(/\/$/) ? '' : '/'), // make sure no double-'/' if opts.talk already ends with '/' - 'embed/stream?asset_url=', - encodeURIComponent(asset_url) - ]; + 'embed/stream?' + ].join(''); - if (comment) { - iframeArray.push('&comment_id='); - iframeArray.push(encodeURIComponent(comment)); - } + url += stringify(query); - if (asset_id) { - iframeArray.push('&asset_id='); - iframeArray.push(encodeURIComponent(asset_id)); - } - - return iframeArray.join(''); + return url; } // Set up postMessage listeners/handlers on the pymParent // e.g. to resize the iframe, and navigate the host page -function configurePymParent(pymParent, asset_url) { +function configurePymParent(pymParent) { let notificationOffset = 200; let ready = false; let cachedHeight; @@ -117,8 +110,8 @@ function configurePymParent(pymParent, asset_url) { if (ready) { window.clearInterval(interval); - // @todo - It's weird to me that this is sent here in addition to the iframe URL. Could it just be in one place? - pymParent.sendMessage('DOMContentLoaded', asset_url); + // TODO: It's weird to me that this is sent here + pymParent.sendMessage('DOMContentLoaded'); } }, 100); }); @@ -166,42 +159,39 @@ Talk.render = function (el, opts) { } opts = opts || {}; - // @todo infer this URL without explicit user input (if possible, may have to be added at build/render time of this script) + // TODO: infer this URL without explicit user input (if possible, may have to be added at build/render time of this script) if (!opts.talk) { throw new Error('Coral.Talk.render() expects opts.talk as the Talk Base URL'); } - // ensure el has an id, as pym can't directly accept the HTMLElement + // Ensure el has an id, as pym can't directly accept the HTMLElement. if (!el.id) { el.id = `_${Math.random()}`; } - let asset_url = opts.asset_url; - if (!asset_url) { - try { - asset_url = document.querySelector('link[rel="canonical"]').href; - } catch (e) { - console.warn('This page does not include a canonical link tag. Talk has inferred this asset_url from the window object. Query params have been stripped, which may cause a single thread to be present across multiple pages.'); - asset_url = window.location.origin + window.location.pathname; + // Compose the query to send down to the Talk API so it knows what to load. + let query = {}; + + query.comment_id = window.location.hash.slice(1); + query.asset_id = opts.asset_id; + + if (!query.asset_id) { + query.asset_url = opts.asset_url; + if (!query.asset_url && !query.asset_id) { + try { + query.asset_url = document.querySelector('link[rel="canonical"]').href; + } catch (e) { + console.warn('This page does not include a canonical link tag. Talk has inferred this asset_url from the window object. Query params have been stripped, which may cause a single thread to be present across multiple pages.'); + query.asset_url = window.location.origin + window.location.pathname; + } } } - let comment = window.location.hash.slice(1); - - let query = { + configurePymParent(new pym.Parent(el.id, buildStreamIframeUrl(opts.talk, query), { title: opts.title, - asset_url: asset_url, id: `${el.id}_iframe`, name: `${el.id}_iframe` - }; - - if (opts.asset_id && opts.asset_id.length > 0) { - query.asset_id = opts.asset_id; - } - - let pymParent = new pym.Parent(el.id, buildStreamIframeUrl(opts.talk, asset_url, comment), query); - - configurePymParent(pymParent, asset_url); + })); }; export default Coral; diff --git a/client/coral-framework/graphql/queries/streamQuery.graphql b/client/coral-framework/graphql/queries/streamQuery.graphql index c9f9d3d5b..102c656e5 100644 --- a/client/coral-framework/graphql/queries/streamQuery.graphql +++ b/client/coral-framework/graphql/queries/streamQuery.graphql @@ -1,6 +1,6 @@ #import "../fragments/commentView.graphql" -query AssetQuery($asset_id: ID, $asset_url: String!, $comment_id: ID!, $has_comment: Boolean!) { +query AssetQuery($asset_id: ID, $asset_url: String, $comment_id: ID!, $has_comment: Boolean!) { # the comment here is for loading one comment and it's children, probably after following a permalink # $has_comment is derived from the comment_id query param in the iframe url, # which is in turn pulled from the host page url diff --git a/graph/resolvers/root_query.js b/graph/resolvers/root_query.js index 6f4151ff6..32e2e4263 100644 --- a/graph/resolvers/root_query.js +++ b/graph/resolvers/root_query.js @@ -8,11 +8,6 @@ const RootQuery = { }, asset(_, query, {loaders: {Assets}}) { if (query.id) { - - // TODO: we may not always have a comment stream here, therefore, when we - // load it, we may also need to create with the url. This may also have to - // move the logic over to the mutators function as an upsert operation - // possibly. return Assets.getByID.load(query.id); } diff --git a/routes/assets/index.js b/routes/assets/index.js index e8c508b52..a5c0eca50 100644 --- a/routes/assets/index.js +++ b/routes/assets/index.js @@ -14,6 +14,7 @@ router.get('/id/:asset_id', (req, res, next) => { } res.render('article', { title: asset.title, + asset_id: asset.id, asset_url: asset.url, body: '', basePath: '/client/embed/stream' diff --git a/routes/index.js b/routes/index.js index 58c8f13d7..0ed2d9dcf 100644 --- a/routes/index.js +++ b/routes/index.js @@ -16,6 +16,7 @@ if (process.env.NODE_ENV !== 'production') { return res.render('article', { title: 'Coral Talk', asset_url: '', + asset_id: '', body: '', basePath: '/client/embed/stream' }); diff --git a/views/article.ejs b/views/article.ejs index 5f90cdf4d..e20f91117 100644 --- a/views/article.ejs +++ b/views/article.ejs @@ -27,7 +27,8 @@ diff --git a/views/embed-stream.ejs b/views/embed-stream.ejs deleted file mode 100644 index c69773bcb..000000000 --- a/views/embed-stream.ejs +++ /dev/null @@ -1,48 +0,0 @@ - - - - - - Talk - Coral Admin - - - - - - -
- - - - From 2546902bcf1f28cc423bb8a06be891141d4470f3 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Mon, 3 Apr 2017 16:59:31 -0600 Subject: [PATCH 02/12] Still grab the url when we have the id --- client/coral-embed/src/index.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/client/coral-embed/src/index.js b/client/coral-embed/src/index.js index 9b1b29879..21c176500 100644 --- a/client/coral-embed/src/index.js +++ b/client/coral-embed/src/index.js @@ -175,15 +175,13 @@ Talk.render = function (el, opts) { query.comment_id = window.location.hash.slice(1); query.asset_id = opts.asset_id; - if (!query.asset_id) { - query.asset_url = opts.asset_url; - if (!query.asset_url && !query.asset_id) { - try { - query.asset_url = document.querySelector('link[rel="canonical"]').href; - } catch (e) { - console.warn('This page does not include a canonical link tag. Talk has inferred this asset_url from the window object. Query params have been stripped, which may cause a single thread to be present across multiple pages.'); - query.asset_url = window.location.origin + window.location.pathname; - } + query.asset_url = opts.asset_url; + if (!query.asset_url) { + try { + query.asset_url = document.querySelector('link[rel="canonical"]').href; + } catch (e) { + console.warn('This page does not include a canonical link tag. Talk has inferred this asset_url from the window object. Query params have been stripped, which may cause a single thread to be present across multiple pages.'); + query.asset_url = window.location.origin + window.location.pathname; } } From e4e2ff03b3f00c76e29f2a4998bd2d198294e77f Mon Sep 17 00:00:00 2001 From: Riley Davis Date: Wed, 5 Apr 2017 14:41:06 -0600 Subject: [PATCH 03/12] only refetch the comments if you're an admin --- client/coral-embed-stream/src/Embed.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/client/coral-embed-stream/src/Embed.js b/client/coral-embed-stream/src/Embed.js index 3f3c395f8..c393df6f0 100644 --- a/client/coral-embed-stream/src/Embed.js +++ b/client/coral-embed-stream/src/Embed.js @@ -41,9 +41,10 @@ class Embed extends Component { state = {activeTab: 0, showSignInDialog: false, activeReplyBox: ''}; changeTab = (tab) => { + const {isAdmin} = this.props.auth; // Everytime the comes from another tab, the Stream needs to be updated. - if (tab === 0) { + if (tab === 0 && isAdmin) { this.props.data.refetch(); } From d32c2f5177231f45ee819fb606b62ccaa5bb37d5 Mon Sep 17 00:00:00 2001 From: Chi Vinh Le Date: Thu, 6 Apr 2017 16:44:50 +0700 Subject: [PATCH 04/12] Fix collapsing comment tree --- client/coral-embed-stream/src/Embed.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/coral-embed-stream/src/Embed.js b/client/coral-embed-stream/src/Embed.js index 3f3c395f8..89c868534 100644 --- a/client/coral-embed-stream/src/Embed.js +++ b/client/coral-embed-stream/src/Embed.js @@ -116,7 +116,7 @@ class Embed extends Component { render () { const {activeTab} = this.state; const {closedAt, countCache = {}} = this.props.asset; - const {loading, asset, refetch, comment} = this.props.data; + const {asset, refetch, comment} = this.props.data; const {loggedIn, isAdmin, user, showSignInDialog, signInOffset} = this.props.auth; // even though the permalinked comment is the highlighted one, we're displaying its parent + replies @@ -130,7 +130,7 @@ class Embed extends Component { minHeight: document.body.scrollHeight + 200 } : {}; - if (loading || !asset) { + if (!asset) { return ; } From 79d5c28d00555b327a9a5e5a8bd99b7d3500dddd Mon Sep 17 00:00:00 2001 From: David Erwin Date: Thu, 6 Apr 2017 10:47:56 -0400 Subject: [PATCH 05/12] Pass asset_id in title route --- routes/assets/index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/routes/assets/index.js b/routes/assets/index.js index a5c0eca50..54d4e72ed 100644 --- a/routes/assets/index.js +++ b/routes/assets/index.js @@ -27,6 +27,7 @@ router.get('/title/:asset_title', (req, res) => { return res.render('article', { title: req.params.asset_title.split('-').join(' '), asset_url: '', + asset_id: null, body: body, basePath: '/client/embed/stream' }); From 1f367e666b116f2c7db4afef5292d242bf60c990 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 6 Apr 2017 10:39:36 -0600 Subject: [PATCH 06/12] Added joi plugin validation --- app.js | 3 +++ graph/context.js | 3 +++ graph/hooks.js | 5 +++++ graph/loaders/index.js | 3 +++ graph/mutators/index.js | 3 +++ graph/resolvers/index.js | 3 +++ graph/typeDefs.js | 3 +++ package.json | 1 + routes/index.js | 3 +++ yarn.lock | 27 +++++++++++++++++++++++++++ 10 files changed, 54 insertions(+) diff --git a/app.js b/app.js index e1e864b10..3c9142da5 100644 --- a/app.js +++ b/app.js @@ -10,6 +10,7 @@ const enabled = require('debug').enabled; const RedisStore = require('connect-redis')(session); const redis = require('./services/redis'); const csrf = require('csurf'); +const Joi = require('joi'); const errors = require('./errors'); const graph = require('./graph'); const apollo = require('graphql-server-express'); @@ -80,6 +81,8 @@ const passportDebug = require('debug')('talk:passport'); // Install the passport plugins. plugins.get('server', 'passport').forEach((plugin) => { + Joi.assert(plugin.passport, Joi.func().arity(1), `Plugin '${plugin.name}' had an error loading the passport hook`); + passportDebug(`added plugin '${plugin.plugin.name}'`); // Pass the passport.js instance to the plugin to allow it to inject it's diff --git a/graph/context.js b/graph/context.js index f3ed42ad8..030647bf7 100644 --- a/graph/context.js +++ b/graph/context.js @@ -3,6 +3,7 @@ const mutators = require('./mutators'); const plugins = require('../services/plugins'); const debug = require('debug')('talk:graph:context'); +const Joi = require('joi'); /** * Contains the array of plugins that provide context to the server, these top @@ -10,6 +11,8 @@ const debug = require('debug')('talk:graph:context'); * @type {Array} */ const contextPlugins = plugins.get('server', 'context').map(({plugin, context}) => { + Joi.assert(context, Joi.object().pattern(/\w/, Joi.func().maxArity(1)), `Plugin '${plugin.name}' had an error loading the context`); + debug(`added plugin '${plugin.name}'`); return {context}; }); diff --git a/graph/hooks.js b/graph/hooks.js index 3fe1306cf..4977c796e 100644 --- a/graph/hooks.js +++ b/graph/hooks.js @@ -1,5 +1,6 @@ const {forEachField} = require('graphql-tools'); const debug = require('debug')('talk:graph:schema'); +const Joi = require('joi'); /** * XXX taken from graphql-js: src/execution/execute.js, because that function @@ -46,6 +47,10 @@ const decorateWithHooks = (schema, hooks) => forEachField(schema, (field, typeNa // Combine the pre/post hooks from each plugin into an array we can // execute. .reduce((acc, {plugin, hooks}) => { + Joi.assert(hooks, Joi.object({ + pre: Joi.func(), + post: Joi.func() + }), `Plugin '${plugin.name}' had an error loading the hooks`); // Itterate over the hooks on the fields and look at it with a switch // block to check for misconfigured plugins. diff --git a/graph/loaders/index.js b/graph/loaders/index.js index 26727940f..3c3ccef2b 100644 --- a/graph/loaders/index.js +++ b/graph/loaders/index.js @@ -1,4 +1,5 @@ const _ = require('lodash'); +const Joi = require('joi'); const debug = require('debug')('talk:graph:loaders'); const Actions = require('./actions'); @@ -23,6 +24,8 @@ let loaders = [ // Load the plugin loaders from the manager. ...plugins .get('server', 'loaders').map(({plugin, loaders}) => { + Joi.assert(loaders, Joi.object().pattern(/\w/, Joi.object().pattern(/\w/, Joi.func())), `Plugin '${plugin.name}' had an error loading the loaders`); + debug(`added plugin '${plugin.name}'`); return loaders; diff --git a/graph/mutators/index.js b/graph/mutators/index.js index 9975bedde..8fc065514 100644 --- a/graph/mutators/index.js +++ b/graph/mutators/index.js @@ -1,4 +1,5 @@ const _ = require('lodash'); +const Joi = require('joi'); const debug = require('debug')('talk:graph:mutators'); const Comment = require('./comment'); @@ -17,6 +18,8 @@ let mutators = [ // Load the plugin mutators from the manager. ...plugins .get('server', 'mutators').map(({plugin, mutators}) => { + Joi.assert(mutators, Joi.object().pattern(/\w/, Joi.object().pattern(/\w/, Joi.func())), `Plugin '${plugin.name}' had an error loading the mutators`); + debug(`added plugin '${plugin.name}'`); return mutators; diff --git a/graph/resolvers/index.js b/graph/resolvers/index.js index 05d633547..743e32062 100644 --- a/graph/resolvers/index.js +++ b/graph/resolvers/index.js @@ -1,4 +1,5 @@ const _ = require('lodash'); +const Joi = require('joi'); const debug = require('debug')('talk:graph:resolvers'); const ActionSummary = require('./action_summary'); @@ -50,6 +51,8 @@ let resolvers = { * as provide new ones. */ resolvers = plugins.get('server', 'resolvers').reduce((acc, {plugin, resolvers}) => { + Joi.assert(resolvers, Joi.object().pattern(/\w/, Joi.object().pattern(/\w/, Joi.func())), `Plugin '${plugin.name}' had an error loading the resolvers`); + debug(`added plugin '${plugin.name}'`); return _.merge(acc, resolvers); diff --git a/graph/typeDefs.js b/graph/typeDefs.js index 9b2c8b7aa..64ee07f6d 100644 --- a/graph/typeDefs.js +++ b/graph/typeDefs.js @@ -3,6 +3,7 @@ // this change is done now everything will likely break on the front end. const fs = require('fs'); +const Joi = require('joi'); const path = require('path'); const {mergeStrings} = require('gql-merge'); const debug = require('debug')('talk:graph:typeDefs'); @@ -20,6 +21,8 @@ const typeDefs = mergeStrings([ // Load the plugin definitions from the manager. ...plugins.get('server', 'typeDefs').map(({plugin, typeDefs}) => { + Joi.assert(typeDefs, Joi.string(), `Plugin '${plugin.name}' had an error loading the typeDefs`); + debug(`added plugin '${plugin.name}'`); return typeDefs; diff --git a/package.json b/package.json index 3ea18dfcd..adff68924 100644 --- a/package.json +++ b/package.json @@ -74,6 +74,7 @@ "graphql-tools": "^0.9.0", "helmet": "^3.5.0", "inquirer": "^3.0.6", + "joi": "^10.4.1", "jsonwebtoken": "^7.3.0", "kue": "^0.11.5", "linkify-it": "^2.0.3", diff --git a/routes/index.js b/routes/index.js index 448bd6964..b7c08694f 100644 --- a/routes/index.js +++ b/routes/index.js @@ -1,5 +1,6 @@ const express = require('express'); const path = require('path'); +const Joi = require('joi'); const plugins = require('../services/plugins'); const debug = require('debug')('talk:routes'); @@ -27,6 +28,8 @@ if (process.env.NODE_ENV !== 'production') { // Inject server route plugins. plugins.get('server', 'router').forEach((plugin) => { + Joi.assert(plugin.router, Joi.func().arity(1), `Plugin '${plugin.name}' had an error loading the passport router`); + debug(`added plugin '${plugin.plugin.name}'`); // Pass the root router to the plugin to mount it's routes. diff --git a/yarn.lock b/yarn.lock index c9e0c2463..4d1a4ed9c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3567,6 +3567,10 @@ hoek@2.x.x: version "2.16.3" resolved "https://registry.yarnpkg.com/hoek/-/hoek-2.16.3.tgz#20bb7403d3cea398e91dc4710a8ff1b8274a25ed" +hoek@4.x.x: + version "4.1.1" + resolved "https://registry.yarnpkg.com/hoek/-/hoek-4.1.1.tgz#9cc573ffba2b7b408fb5e9c2a13796be94cddce9" + hoist-non-react-statics@^1.0.3, hoist-non-react-statics@^1.2.0: version "1.2.0" resolved "https://registry.yarnpkg.com/hoist-non-react-statics/-/hoist-non-react-statics-1.2.0.tgz#aa448cf0986d55cc40773b17174b7dd066cb7cfb" @@ -4097,6 +4101,10 @@ isemail@1.x.x: version "1.2.0" resolved "https://registry.yarnpkg.com/isemail/-/isemail-1.2.0.tgz#be03df8cc3e29de4d2c5df6501263f1fa4595e9a" +isemail@2.x.x: + version "2.2.1" + resolved "https://registry.yarnpkg.com/isemail/-/isemail-2.2.1.tgz#0353d3d9a62951080c262c2aa0a42b8ea8e9e2a6" + isexe@^1.1.1: version "1.1.2" resolved "https://registry.yarnpkg.com/isexe/-/isexe-1.1.2.tgz#36f3e22e60750920f5e7241a476a8c6a42275ad0" @@ -4195,6 +4203,10 @@ istanbul@^1.1.0-alpha.1: which "^1.1.1" wordwrap "^1.0.0" +items@2.x.x: + version "2.1.1" + resolved "https://registry.yarnpkg.com/items/-/items-2.1.1.tgz#8bd16d9c83b19529de5aea321acaada78364a198" + iterall@1.0.2: version "1.0.2" resolved "https://registry.yarnpkg.com/iterall/-/iterall-1.0.2.tgz#41a2e96ce9eda5e61c767ee5dc312373bb046e91" @@ -4212,6 +4224,15 @@ jodid25519@^1.0.0: dependencies: jsbn "~0.1.0" +joi@^10.4.1: + version "10.4.1" + resolved "https://registry.yarnpkg.com/joi/-/joi-10.4.1.tgz#a2fca1f0d603d1b843f2c1e086b52461f6be1f36" + dependencies: + hoek "4.x.x" + isemail "2.x.x" + items "2.x.x" + topo "2.x.x" + joi@^6.10.1: version "6.10.1" resolved "https://registry.yarnpkg.com/joi/-/joi-6.10.1.tgz#4d50c318079122000fe5f16af1ff8e1917b77e06" @@ -7658,6 +7679,12 @@ topo@1.x.x: dependencies: hoek "2.x.x" +topo@2.x.x: + version "2.0.2" + resolved "https://registry.yarnpkg.com/topo/-/topo-2.0.2.tgz#cd5615752539057c0dc0491a621c3bc6fbe1d182" + dependencies: + hoek "4.x.x" + touch@1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/touch/-/touch-1.0.0.tgz#449cbe2dbae5a8c8038e30d71fa0ff464947c4de" From 28ce77a00220036d2c73c30d3c38e21a1ab60151 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 6 Apr 2017 10:56:32 -0600 Subject: [PATCH 07/12] Moved validation to one place --- app.js | 3 --- graph/context.js | 3 --- graph/hooks.js | 5 ----- graph/loaders/index.js | 3 --- graph/mutators/index.js | 3 --- graph/resolvers/index.js | 3 --- graph/typeDefs.js | 3 --- plugins.js | 24 ++++++++++++++++++++++++ routes/index.js | 3 --- 9 files changed, 24 insertions(+), 26 deletions(-) diff --git a/app.js b/app.js index 3c9142da5..e1e864b10 100644 --- a/app.js +++ b/app.js @@ -10,7 +10,6 @@ const enabled = require('debug').enabled; const RedisStore = require('connect-redis')(session); const redis = require('./services/redis'); const csrf = require('csurf'); -const Joi = require('joi'); const errors = require('./errors'); const graph = require('./graph'); const apollo = require('graphql-server-express'); @@ -81,8 +80,6 @@ const passportDebug = require('debug')('talk:passport'); // Install the passport plugins. plugins.get('server', 'passport').forEach((plugin) => { - Joi.assert(plugin.passport, Joi.func().arity(1), `Plugin '${plugin.name}' had an error loading the passport hook`); - passportDebug(`added plugin '${plugin.plugin.name}'`); // Pass the passport.js instance to the plugin to allow it to inject it's diff --git a/graph/context.js b/graph/context.js index 030647bf7..f3ed42ad8 100644 --- a/graph/context.js +++ b/graph/context.js @@ -3,7 +3,6 @@ const mutators = require('./mutators'); const plugins = require('../services/plugins'); const debug = require('debug')('talk:graph:context'); -const Joi = require('joi'); /** * Contains the array of plugins that provide context to the server, these top @@ -11,8 +10,6 @@ const Joi = require('joi'); * @type {Array} */ const contextPlugins = plugins.get('server', 'context').map(({plugin, context}) => { - Joi.assert(context, Joi.object().pattern(/\w/, Joi.func().maxArity(1)), `Plugin '${plugin.name}' had an error loading the context`); - debug(`added plugin '${plugin.name}'`); return {context}; }); diff --git a/graph/hooks.js b/graph/hooks.js index 4977c796e..3fe1306cf 100644 --- a/graph/hooks.js +++ b/graph/hooks.js @@ -1,6 +1,5 @@ const {forEachField} = require('graphql-tools'); const debug = require('debug')('talk:graph:schema'); -const Joi = require('joi'); /** * XXX taken from graphql-js: src/execution/execute.js, because that function @@ -47,10 +46,6 @@ const decorateWithHooks = (schema, hooks) => forEachField(schema, (field, typeNa // Combine the pre/post hooks from each plugin into an array we can // execute. .reduce((acc, {plugin, hooks}) => { - Joi.assert(hooks, Joi.object({ - pre: Joi.func(), - post: Joi.func() - }), `Plugin '${plugin.name}' had an error loading the hooks`); // Itterate over the hooks on the fields and look at it with a switch // block to check for misconfigured plugins. diff --git a/graph/loaders/index.js b/graph/loaders/index.js index 3c3ccef2b..26727940f 100644 --- a/graph/loaders/index.js +++ b/graph/loaders/index.js @@ -1,5 +1,4 @@ const _ = require('lodash'); -const Joi = require('joi'); const debug = require('debug')('talk:graph:loaders'); const Actions = require('./actions'); @@ -24,8 +23,6 @@ let loaders = [ // Load the plugin loaders from the manager. ...plugins .get('server', 'loaders').map(({plugin, loaders}) => { - Joi.assert(loaders, Joi.object().pattern(/\w/, Joi.object().pattern(/\w/, Joi.func())), `Plugin '${plugin.name}' had an error loading the loaders`); - debug(`added plugin '${plugin.name}'`); return loaders; diff --git a/graph/mutators/index.js b/graph/mutators/index.js index 8fc065514..9975bedde 100644 --- a/graph/mutators/index.js +++ b/graph/mutators/index.js @@ -1,5 +1,4 @@ const _ = require('lodash'); -const Joi = require('joi'); const debug = require('debug')('talk:graph:mutators'); const Comment = require('./comment'); @@ -18,8 +17,6 @@ let mutators = [ // Load the plugin mutators from the manager. ...plugins .get('server', 'mutators').map(({plugin, mutators}) => { - Joi.assert(mutators, Joi.object().pattern(/\w/, Joi.object().pattern(/\w/, Joi.func())), `Plugin '${plugin.name}' had an error loading the mutators`); - debug(`added plugin '${plugin.name}'`); return mutators; diff --git a/graph/resolvers/index.js b/graph/resolvers/index.js index 743e32062..05d633547 100644 --- a/graph/resolvers/index.js +++ b/graph/resolvers/index.js @@ -1,5 +1,4 @@ const _ = require('lodash'); -const Joi = require('joi'); const debug = require('debug')('talk:graph:resolvers'); const ActionSummary = require('./action_summary'); @@ -51,8 +50,6 @@ let resolvers = { * as provide new ones. */ resolvers = plugins.get('server', 'resolvers').reduce((acc, {plugin, resolvers}) => { - Joi.assert(resolvers, Joi.object().pattern(/\w/, Joi.object().pattern(/\w/, Joi.func())), `Plugin '${plugin.name}' had an error loading the resolvers`); - debug(`added plugin '${plugin.name}'`); return _.merge(acc, resolvers); diff --git a/graph/typeDefs.js b/graph/typeDefs.js index 64ee07f6d..9b2c8b7aa 100644 --- a/graph/typeDefs.js +++ b/graph/typeDefs.js @@ -3,7 +3,6 @@ // this change is done now everything will likely break on the front end. const fs = require('fs'); -const Joi = require('joi'); const path = require('path'); const {mergeStrings} = require('gql-merge'); const debug = require('debug')('talk:graph:typeDefs'); @@ -21,8 +20,6 @@ const typeDefs = mergeStrings([ // Load the plugin definitions from the manager. ...plugins.get('server', 'typeDefs').map(({plugin, typeDefs}) => { - Joi.assert(typeDefs, Joi.string(), `Plugin '${plugin.name}' had an error loading the typeDefs`); - debug(`added plugin '${plugin.name}'`); return typeDefs; diff --git a/plugins.js b/plugins.js index d7c9f9426..9120c8d71 100644 --- a/plugins.js +++ b/plugins.js @@ -2,6 +2,7 @@ const fs = require('fs'); const path = require('path'); const resolve = require('resolve'); const debug = require('debug')('talk:plugins'); +const Joi = require('joi'); // Add support for require rewriting. require('app-module-path').addPath(__dirname); @@ -21,6 +22,20 @@ try { } } +const hookSchemas = { + passport: Joi.func().arity(1), + router: Joi.func().arity(1), + context: Joi.object().pattern(/\w/, Joi.func().maxArity(1)), + hooks: Joi.object({ + pre: Joi.func(), + post: Joi.func() + }), + loaders: Joi.object().pattern(/\w/, Joi.object().pattern(/\w/, Joi.func())), + mutators: Joi.object().pattern(/\w/, Joi.object().pattern(/\w/, Joi.func())), + resolvers: Joi.object().pattern(/\w/, Joi.object().pattern(/\w/, Joi.func())), + typeDefs: Joi.string() +}; + /** * isInternal checks to see if a given plugin is internal, and returns true * if it is. @@ -122,6 +137,15 @@ class PluginSection { hook(hook) { return this.plugins .filter(({module}) => hook in module) + .filter((plugin) => { + + // Validate the hook. + if (hook in hookSchemas) { + Joi.assert(plugin.module[hook], hookSchemas[hook], `Plugin '${plugin.name}' failed validation for the '${hook}' hook`); + } + + return true; + }) .map((plugin) => ({ plugin, [hook]: plugin.module[hook] diff --git a/routes/index.js b/routes/index.js index b7c08694f..448bd6964 100644 --- a/routes/index.js +++ b/routes/index.js @@ -1,6 +1,5 @@ const express = require('express'); const path = require('path'); -const Joi = require('joi'); const plugins = require('../services/plugins'); const debug = require('debug')('talk:routes'); @@ -28,8 +27,6 @@ if (process.env.NODE_ENV !== 'production') { // Inject server route plugins. plugins.get('server', 'router').forEach((plugin) => { - Joi.assert(plugin.router, Joi.func().arity(1), `Plugin '${plugin.name}' had an error loading the passport router`); - debug(`added plugin '${plugin.plugin.name}'`); // Pass the root router to the plugin to mount it's routes. From fb8b33b2eb40c1db25d87402c36197a0556a2762 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 6 Apr 2017 11:43:18 -0600 Subject: [PATCH 08/12] Added plugins.default.json --- .gitignore | 1 + plugins.json => plugins.default.json | 0 plugins.js | 13 +++++++++++-- 3 files changed, 12 insertions(+), 2 deletions(-) rename plugins.json => plugins.default.json (100%) diff --git a/.gitignore b/.gitignore index 4ad15747d..6b7b187d9 100644 --- a/.gitignore +++ b/.gitignore @@ -14,5 +14,6 @@ dump.rdb test/e2e/reports coverage/ +plugins.json plugins/* !plugins/coral-plugin-facebook-auth \ No newline at end of file diff --git a/plugins.json b/plugins.default.json similarity index 100% rename from plugins.json rename to plugins.default.json diff --git a/plugins.js b/plugins.js index d7c9f9426..4483f0595 100644 --- a/plugins.js +++ b/plugins.js @@ -12,10 +12,19 @@ let plugins = {}; // file isn't loaded, but continuing. Else, like a parsing error, throw it and // crash the program. try { - plugins = JSON.parse(fs.readFileSync(path.join(__dirname, 'plugins.json'), 'utf8')); + let defaultPlugins = path.join(__dirname, 'plugins.default.json'); + let customPlugins = path.join(__dirname, 'plugins.json'); + + if (fs.existsSync(customPlugins)) { + debug(`Now using ${customPlugins} for plugins`); + plugins = JSON.parse(fs.readFileSync(customPlugins, 'utf8')); + } else { + debug(`Now using ${defaultPlugins} for plugins`); + plugins = JSON.parse(fs.readFileSync(defaultPlugins, 'utf8')); + } } catch (err) { if (err.code === 'ENOENT') { - console.error('plugins.json not found, plugins will not be active'); + console.error('plugins.json and plguins.default.json not found, plugins will not be active'); } else { throw err; } From a9c88f52cec4f796adcb3c135f688ac00385e389 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 6 Apr 2017 11:58:18 -0600 Subject: [PATCH 09/12] Optionally load plugins config from environment --- PLUGINS.md | 15 +++++++++++---- README.md | 1 + plugins.js | 6 +++++- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/PLUGINS.md b/PLUGINS.md index e5cf8d760..e0e99cccd 100644 --- a/PLUGINS.md +++ b/PLUGINS.md @@ -3,11 +3,18 @@ Plugins for Talk can take various forms, currently we are only supporting server side plugins. -## Plugin Registration: `plugins.json` +## Plugin Registration -All plugins must be registered in the root file `plugins.json`. +The parsing order for the plugin regsitration is as follows: -The format for this file is thus: +- `process.env.TALK_PLUGINS_JSON` +- `plugins.json` +- `plugins.default.json` + +If you need to "disable all plugins", you can simply provide `{}` as the +contents of `process.env.TALK_PLUGINS_JSON` or the `plugins.json`. + +The format for this is thus: ```json { @@ -18,7 +25,7 @@ The format for this file is thus: ``` Where we have a `server` key with an array of plugins that match the folder -name in the `plugins/` folder. For example, the above `plugins.json` would +name in the `plugins/` folder. For example, the above config would require a plugin from `plugins/people`, which must provide a `index.js` file that returns an object that matches the Plugin Specification. diff --git a/README.md b/README.md index b88138425..ef621b59c 100644 --- a/README.md +++ b/README.md @@ -37,6 +37,7 @@ available in the format: `://` without the path. - `TALK_INSTALL_LOCK` (_optional for dynamic setup_) - Defaults to `FALSE`. When `TRUE`, disables the dynamic setup endpoint. - `TALK_RECAPTCHA_SECRET` (*required for reCAPTCHA support*) - server secret used for enabling reCAPTCHA powered logins. If not provided it will instead default to providing only a time based lockout. - `TALK_RECAPTCHA_PUBLIC` (*required for reCAPTCHA support*) - client secret used for enabling reCAPTCHA powered logins. If not provided it will instead default to providing only a time based lockout. +- `TALK_PLUGINS_JSON` (_optional_) - used to specify the plugin config via the environment Refer to the wiki page on [Configuration Loading](https://github.com/coralproject/talk/wiki/Configuration-Loading) for alternative methods of loading configuration during development. diff --git a/plugins.js b/plugins.js index 4483f0595..f9d93bdce 100644 --- a/plugins.js +++ b/plugins.js @@ -14,8 +14,12 @@ let plugins = {}; try { let defaultPlugins = path.join(__dirname, 'plugins.default.json'); let customPlugins = path.join(__dirname, 'plugins.json'); + let envPluginJSON = process.env.TALK_PLUGINS_JSON; - if (fs.existsSync(customPlugins)) { + if (envPluginJSON && envPluginJSON.length > 0) { + debug('Now using TALK_PLUGINS_JSON environment variable for plugins'); + plugins = JSON.parse(envPluginJSON); + } else if (fs.existsSync(customPlugins)) { debug(`Now using ${customPlugins} for plugins`); plugins = JSON.parse(fs.readFileSync(customPlugins, 'utf8')); } else { From 943d4cf98a1c348f4d78d7eb5aac870dfcd54cd0 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 6 Apr 2017 12:00:00 -0600 Subject: [PATCH 10/12] Spelling --- PLUGINS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/PLUGINS.md b/PLUGINS.md index e0e99cccd..c34cfa5eb 100644 --- a/PLUGINS.md +++ b/PLUGINS.md @@ -5,7 +5,7 @@ side plugins. ## Plugin Registration -The parsing order for the plugin regsitration is as follows: +The parsing order for the plugin registration is as follows: - `process.env.TALK_PLUGINS_JSON` - `plugins.json` From 4bc7848e24cdfc572531f8cb9d8355ae40ca6bdb Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 6 Apr 2017 12:00:36 -0600 Subject: [PATCH 11/12] comment updates --- PLUGINS.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/PLUGINS.md b/PLUGINS.md index c34cfa5eb..8bf00229d 100644 --- a/PLUGINS.md +++ b/PLUGINS.md @@ -7,9 +7,9 @@ side plugins. The parsing order for the plugin registration is as follows: -- `process.env.TALK_PLUGINS_JSON` -- `plugins.json` -- `plugins.default.json` +- `TALK_PLUGINS_JSON` environment variable +- `plugins.json` file +- `plugins.default.json` file If you need to "disable all plugins", you can simply provide `{}` as the contents of `process.env.TALK_PLUGINS_JSON` or the `plugins.json`. From 770a4e6fe2a1b96631187e4a17417165df767068 Mon Sep 17 00:00:00 2001 From: Kim Gardner Date: Thu, 6 Apr 2017 14:38:16 -0400 Subject: [PATCH 12/12] Fix typo --- plugins.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins.js b/plugins.js index f9d93bdce..43ac04478 100644 --- a/plugins.js +++ b/plugins.js @@ -28,7 +28,7 @@ try { } } catch (err) { if (err.code === 'ENOENT') { - console.error('plugins.json and plguins.default.json not found, plugins will not be active'); + console.error('plugins.json and plugins.default.json not found, plugins will not be active'); } else { throw err; }