From d3219f03ad30286e00aca2bd4d6d88ec4cabfa83 Mon Sep 17 00:00:00 2001
From: Remy Sharp <remy@remysharp.com>
Date: Wed, 24 Aug 2016 15:48:14 +0100
Subject: [PATCH] feat: support private & public rulesets

BREAKING CHANGE:

The accept.json file format has now changes from being rooted as an array to being an object with `private` and `public` accept rules.
---
 README.md                             | 11 +++---
 lib/client/index.js                   | 13 +++---
 lib/client/socket.js                  |  8 ++--
 lib/filters/index.js                  | 18 ++++++---
 lib/index.js                          | 17 +++++---
 lib/relay.js                          | 57 +++++++++++++++------------
 lib/server/index.js                   | 20 ++++++----
 lib/server/socket.js                  |  4 +-
 test/fixtures/client/filters.json     | 28 +++++++++----
 test/fixtures/relay.json              |  8 ++++
 test/fixtures/server/filters.json     | 31 +++++++++++----
 test/functional/client-server.test.js | 18 +++++----
 test/functional/index.test.js         | 19 +++++----
 test/functional/no-filter.test.js     | 15 ++++---
 test/unit/filters.test.js             |  2 +-
 test/utils.js                         |  7 +++-
 16 files changed, 176 insertions(+), 100 deletions(-)

diff --git a/README.md b/README.md
index 03652a6f6..8e389bdfa 100644
--- a/README.md
+++ b/README.md
@@ -99,12 +99,13 @@ The second, `${PARAM}` is populated with the matching value in your configuratio
 
 The final result is that the broker will accept and forward `GET` requests to my local server that will respond to `https://12345678@foo-bar.com/snyk/broker/master/package.json`.
 
-# TODO / Aims
+### private
 
-- [x] Proxy e2e socket (server -> client -> internal -> client -> server)
-- [x] Can serve as both client and server
-- [x] Client can forward requests from internal to server
-- [ ] Filter relays (i.e. whether the local webserver should accept the inbound request)
+Private filters are for requests that come from the broker server into your client and ask for resources inside your private infrastructure (such as a github enterprise instance).
+
+### public
+
+Public filters are for requests that a recieved on your broker client and are intended to be forwarded to the broker server (such as a github webhook).
 
 # Notes
 
diff --git a/lib/client/index.js b/lib/client/index.js
index acd4c5dee..4f32057c4 100644
--- a/lib/client/index.js
+++ b/lib/client/index.js
@@ -2,10 +2,8 @@ const debug = require('debug')('broker:client');
 const socket = require('./socket');
 const relay = require('../relay');
 
-module.exports = ({ port = null }) => {
-  debug('running client');
-  // we import config here to allow the tests to invalidate the require cache
-  const config = require('../config');
+module.exports = ({ port = null, config = {}, filters = {} }) => {
+  debug('running');
 
   // start the local webserver to listen for relay requests
   const { app, server } = require('../webserver')(config, port);
@@ -13,19 +11,20 @@ module.exports = ({ port = null }) => {
   const io = socket({
     id: config.brokerId,
     url: config.brokerUrl,
+    filters: filters.private,
   });
 
   app.all('/*', (req, res, next) => {
     res.locals.io = io;
     next();
-  }, relay.request);
+  }, relay.request(filters.public));
 
   return {
     io,
-    close: () => {
+    close: done => {
       debug('closing');
       server.close();
-      io.end();
+      io.destroy(done || (() => debug('closed')));
     },
   };
 };
diff --git a/lib/client/socket.js b/lib/client/socket.js
index 2e43d965f..4cf110c1f 100644
--- a/lib/client/socket.js
+++ b/lib/client/socket.js
@@ -1,6 +1,6 @@
 const debug = require('debug')('broker:client');
-const relay = require('../relay');
 const Primus = require('primus');
+const relay = require('../relay');
 const httpErrors = require('../http-errors');
 const Socket = Primus.createSocket({
   transformer: 'engine.io',
@@ -10,7 +10,7 @@ const Socket = Primus.createSocket({
   }
 });
 
-module.exports = ({ url, id }) => {
+module.exports = ({ url, id, filters }) => {
   if (!id) { // null, undefined, empty, etc.
     debug('missing client id');
     const error = new ReferenceError('BROKER_ID is required to successfully identify itself to the server');
@@ -29,12 +29,12 @@ module.exports = ({ url, id }) => {
 
   debug('connecting to %s', url);
 
-  const response = relay.response();
+  const response = relay.response(filters);
 
   // RS note: this bind doesn't feel right, it feels like a sloppy way of
   // getting the filters into the request function.
   io.on('request', response);
-  io.on('error', ({ message, type, description }) => {
+  io.on('error', ({ type, description }) => {
     if (type === 'TransportError') {
       console.error(`Failed to connect to broker server: ${httpErrors[description]}`);
     }
diff --git a/lib/filters/index.js b/lib/filters/index.js
index 0f8c74a26..264856b2f 100644
--- a/lib/filters/index.js
+++ b/lib/filters/index.js
@@ -1,17 +1,22 @@
 const pathRegexp = require('path-to-regexp');
-const debug = require('debug')('broker');
 const undefsafe = require('undefsafe');
 
 // reads config that defines
-module.exports = function (filename) {
+module.exports = ruleSource => {
+  const debug = require('debug')('broker:' + (process.env.BROKER_TYPE || 'filter'));
+  debug('loading new rules', ruleSource);
+
   let rules = [];
   const config = require('../config');
 
-  if (filename) {
+  // polymorphic support
+  if (Array.isArray(ruleSource)) {
+    rules = ruleSource;
+  } else if (ruleSource) {
     try {
-      rules = require(filename);
+      rules = require(ruleSource);
     } catch (e) {
-      console.warn(`Unable to parse ${filename}, ignoring for now: ${e.message}`);
+      console.warn(`Unable to parse ${ruleSource}, ignoring for now: ${e.message}`);
     }
   }
 
@@ -55,7 +60,7 @@ module.exports = function (filename) {
       let url = req.url.split('?').shift(); // strip the querystring
       const res = regexp.exec(url);
       if (!res) {
-        // console.log('false', path, req.url, regexp.source);
+        // debug('bad regexp match', path, req.url, regexp.source);
         // no url match
         return false;
       }
@@ -87,6 +92,7 @@ module.exports = function (filename) {
 
   return (url, callback) => {
     let res = false;
+    debug(`testing ${tests.length} rules`);
     for (const test of tests) {
       res = test(url);
       if (res) {
diff --git a/lib/index.js b/lib/index.js
index 1fc63aeef..db93db1eb 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -1,21 +1,28 @@
 require('clarify'); // clean the stacktraces
+const path = require('path');
 
 const app = module.exports = {
   client: require('./client'),
   server: require('./server'),
+  main: main,
 };
 
 function main({ port } = {}) {
   // note: the config is loaded in the main function to allow us to mock in tests
   const config = require('./config');
+  const client = !!config.brokerUrl;
+  const method = client ? 'client' : 'server';
+  process.env.BROKER_TYPE = method;
 
-  // if the config has the broker server, then we must assume it's in client
-  // mode.
-  if (config.brokerUrl) {
-    return app.client(config, port);
+  let filters = {};
+  if (config.accept) {
+    require('debug')(`broker:${method}`)('loading rules from %s', config.accept);
+    filters = require(path.resolve(process.cwd(), config.accept));
   }
 
-  return app.server(config, port);
+
+  // if the config has the broker server, then we must assume it's a client
+  return app[method]({ config, port, filters });
 }
 
 if (!module.parent) {
diff --git a/lib/relay.js b/lib/relay.js
index 0aa8af05f..9704f5c96 100644
--- a/lib/relay.js
+++ b/lib/relay.js
@@ -1,6 +1,5 @@
-const debug = require('debug')('broker');
 const request = require('request');
-const path = require('path');
+const Filters = require('./filters');
 const parse = require('url').parse;
 
 module.exports = {
@@ -8,37 +7,45 @@ module.exports = {
   response: responseHandler,
 };
 
-function requestHandler(req, res) {
-  debug('send socket request for', req.url);
+function requestHandler(filterRules) {
+  const debug = require('debug')('broker:' + (process.env.BROKER_TYPE || 'relay'));
+  const filters = Filters(filterRules);
 
-  // send the socket request containing the http request we're after
-  res.locals.io.send('request', {
-    url: req.url, // strip the leading
-    method: req.method,
-    body: req.body,
-    headers: {
-      'user-agent': req.headers['user-agent'],
-      authorization: req.headers.authorization,
-    },
-  }, response => {
-    console.log('%s %s (%s)', req.method, req.url, response.status);
-    res.status(response.status).send(response.body);
-  });
-}
+  return (req, res) => {
+    filters(req, (error, result) => {
+      if (error) {
+        return res.status(400).send(error);
+      }
 
-function responseHandler(filterPath) {
-  const config = require('./config');
+      const url = parse(result);
+      req.url = url.pathname;
+      debug('send socket request for', req.url);
 
-  if (!filterPath && config.accept) {
-    filterPath = path.resolve(process.cwd(), config.accept);
-  }
+      // send the socket request containing the http request we're after
+      res.locals.io.send('request', {
+        url: req.url, // strip the leading
+        method: req.method,
+        body: req.body,
+        headers: {
+          'user-agent': req.headers['user-agent'],
+          authorization: req.headers.authorization,
+        },
+      }, response => {
+        debug('%s %s (%s)', req.method, req.url, response.status);
+        res.status(response.status).send(response.body);
+      });
+    });
+  };
+}
 
-  const filters = require('./filters')(filterPath);
+function responseHandler(filterRules) {
+  const filters = Filters(filterRules);
+  const debug = require('debug')('broker:' + (process.env.BROKER_TYPE || 'relay'));
 
   return ({ url, headers, method, body = null } = {}, emit) => {
     // run the request through the filter
     debug('request captured', url, method);
-    filters({ url, method }, (error, result) => {
+    filters({ url, method, body }, (error, result) => {
       if (error) {
         debug('blocked %s %s', method, url);
         return emit({
diff --git a/lib/server/index.js b/lib/server/index.js
index 60117c709..1d67aeed6 100644
--- a/lib/server/index.js
+++ b/lib/server/index.js
@@ -2,15 +2,17 @@ const debug = require('debug')('broker:server');
 const socket = require('./socket');
 const relay = require('../relay');
 
-module.exports = function ({ port = null }) {
-  debug('running server');
-  // we import config here to allow the tests to invalidate the require cache
-  const config = require('../config');
+module.exports = ({ config = {}, port = null, filters = {} }) => {
+  debug('running');
 
+  // start the local webserver to listen for relay requests
   const { app, server } = require('../webserver')(config, port);
 
   // bind the socket server to the web server
-  const { io, connections } = socket(server);
+  const { io, connections } = socket({
+    server,
+    filters: filters.private,
+  });
 
   app.all('/broker/:id/*', (req, res, next) => {
     const id = req.params.id;
@@ -21,20 +23,22 @@ module.exports = function ({ port = null }) {
       return res.status(404).send(null);
     }
 
+
     res.locals.io = connections.get(id);
 
      // strip the leading url
     req.url = req.url.slice(`/broker/${id}`.length);
+    debug('request for %s', req.url);
 
     next();
-  }, relay.request);
+  }, relay.request(filters.public));
 
   return {
     io,
-    close: () => {
+    close: done => {
       debug('closing');
       server.close();
-      io.end();
+      io.destroy(done || (() => debug('closed')));
     },
   };
 };
diff --git a/lib/server/socket.js b/lib/server/socket.js
index ed5540d01..574742157 100644
--- a/lib/server/socket.js
+++ b/lib/server/socket.js
@@ -3,12 +3,12 @@ const Emitter = require('primus-emitter');
 const debug = require('debug')('broker:server');
 const relay = require('../relay');
 
-module.exports = (server) => {
+module.exports = ({ server, filters }) => {
   const io = new Primus(server, { transformer: 'engine.io', parser: 'JSON' });
   io.plugin('emitter', Emitter);
 
   const connections = new Map();
-  const response = relay.response();
+  const response = relay.response(filters);
 
   io.on('error', error => console.error(error.stack));
   io.on('offline', () => console.error('Internet access has gone offline'));
diff --git a/test/fixtures/client/filters.json b/test/fixtures/client/filters.json
index dbe3d2942..06423af00 100644
--- a/test/fixtures/client/filters.json
+++ b/test/fixtures/client/filters.json
@@ -1,7 +1,21 @@
-[
-  {
-    "path": "/magic-path/${secret}/package.json",
-    "method": "POST",
-    "origin": "http://localhost:${port}"
-  }
-]
+{
+  "private": [
+    {
+      "path": "/magic-path/${secret}/package.json",
+      "method": "POST",
+      "origin": "http://localhost:${port}"
+    }
+  ],
+  "public": [
+    {
+      "path": "/magic-path/${secret}/package.json",
+      "method": "POST",
+      "origin": "http://localhost:${port}"
+    },
+      {
+        "path": "/service/:package",
+        "method": "GET",
+        "origin": "http://localhost:${port}"
+      }
+  ]
+}
diff --git a/test/fixtures/relay.json b/test/fixtures/relay.json
index 78182ab23..1424db33f 100644
--- a/test/fixtures/relay.json
+++ b/test/fixtures/relay.json
@@ -4,10 +4,18 @@
     "method": "POST",
     "path": "/",
     "valid": [
+      {
+        "path": "commits.*.added.*",
+        "value": "package.json"
+      },
       {
         "path": "commits.*.modified.*",
         "value": "package.json"
       },
+      {
+        "path": "commits.*.added.*",
+        "value": ".snyk"
+      },
       {
         "path": "commits.*.modified.*",
         "value": ".snyk"
diff --git a/test/fixtures/server/filters.json b/test/fixtures/server/filters.json
index b81e42dc1..b2db9969b 100644
--- a/test/fixtures/server/filters.json
+++ b/test/fixtures/server/filters.json
@@ -1,7 +1,24 @@
-[
-  {
-    "path": "/service/:package",
-    "method": "GET",
-    "origin": "http://localhost:${port}"
-  }
-]
+{
+  "private": [
+    {
+      "path": "/service/:package",
+      "method": "GET",
+      "origin": "http://localhost:${port}"
+    }
+  ],
+  "public": [
+    {
+      "path": "/service/:package",
+      "method": "GET",
+      "origin": "http://localhost:${port}"
+    },
+
+    {
+      "path": "/magic-path/${secret}/package.json",
+      "method": "POST",
+      "origin": "http://localhost:${port}"
+    }
+
+  ]
+
+}
diff --git a/test/functional/client-server.test.js b/test/functional/client-server.test.js
index 61d8f5a3e..f3a7dcc56 100644
--- a/test/functional/client-server.test.js
+++ b/test/functional/client-server.test.js
@@ -6,7 +6,7 @@ const request = require('request');
 const app = require('../../lib');
 const root = __dirname;
 
-const { port, localPort: servicePort } = require('../utils')(tap);
+const { port, localPort: servicePort, resetConfig } = require('../utils')(tap);
 
 test('internal sends request through client', t => {
 
@@ -14,16 +14,18 @@ test('internal sends request through client', t => {
   process.chdir(path.resolve(root, '../fixtures/server'));
   process.env.ACCEPT = 'filters.json';
   process.env.PORT = servicePort;
+  process.env.BROKER_TYPE = 'server';
   const serverPort = port();
-  const server = app.server({ port: serverPort });
+  const server = app.main({ port: serverPort });
 
   process.chdir(path.resolve(root, '../fixtures/client'));
   process.env.BROKER_URL = `http://localhost:${serverPort}`;
   process.env.BROKER_ID = '12345';
+  process.env.BROKER_TYPE = 'client';
   const localPort = port();
   // invalidate the config require
-  delete require.cache[require.resolve(__dirname + '/../../lib/config.js')];
-  const client = app.client({ port: localPort });
+  resetConfig();
+  const client = app.main({ port: localPort });
 
   // wait for the client to successfully connect to the server and identify itself
   server.io.once('connection', socket => {
@@ -40,10 +42,12 @@ test('internal sends request through client', t => {
       });
 
       t.test('clean up', t => {
-        server.close();
         client.close();
-        t.ok('sockets closed');
-        t.end();
+        setTimeout(() => {
+          server.close();
+          t.ok('sockets closed');
+          t.end();
+        }, 100);
       });
     });
   });
diff --git a/test/functional/index.test.js b/test/functional/index.test.js
index fb0f2c227..098267e35 100644
--- a/test/functional/index.test.js
+++ b/test/functional/index.test.js
@@ -19,7 +19,9 @@ test('simple end to end proxying', t => {
 
   process.chdir(path.resolve(root, '../fixtures/server'));
   const serverPort = port();
-  const server = app.server({ port: serverPort });
+  process.env.BROKER_TYPE = 'server';
+  process.env.ACCEPT = 'filters.json';
+  const server = app.main({ port: serverPort });
 
   process.chdir(path.resolve(root, '../fixtures/client'));
   process.env.SECRET = 'secret';
@@ -27,9 +29,10 @@ test('simple end to end proxying', t => {
   process.env.ACCEPT = 'filters.json';
   process.env.BROKER_URL = `http://localhost:${serverPort}`;
   process.env.BROKER_ID = '12345';
+  process.env.BROKER_TYPE = 'client';
   // invalidate the config require
   delete require.cache[require.resolve(__dirname + '/../../lib/config.js')];
-  const client = app.client({ port: port() });
+  const client = app.main({ port: port() });
 
   // wait for the client to successfully connect to the server and identify itself
   server.io.on('connection', socket => {
@@ -48,9 +51,9 @@ test('simple end to end proxying', t => {
 
       t.test('filtered request to broker', t => {
         const url = `http://localhost:${serverPort}/broker/${id}/magic-path/x/random.json`;
-        request({ url, 'method': 'post', json: true }, (err, res) => {
+        request({ url, 'method': 'post', json: true }, (err, res, body) => {
           t.equal(res.statusCode, 400, '400 statusCode');
-          t.match(res.body.toString(), /blocked/, '"blocked" body');
+          t.match(body, /blocked/, '"blocked" body: ' + body);
           t.end();
         });
       });
@@ -64,10 +67,12 @@ test('simple end to end proxying', t => {
       });
 
       t.test('clean up', t => {
-        server.close();
         client.close();
-        t.ok('sockets closed');
-        t.end();
+        setTimeout(() => {
+          server.close();
+          t.ok('sockets closed');
+          t.end();
+        }, 100);
       });
     });
   });
diff --git a/test/functional/no-filter.test.js b/test/functional/no-filter.test.js
index 09004beec..5787f69ed 100644
--- a/test/functional/no-filter.test.js
+++ b/test/functional/no-filter.test.js
@@ -20,7 +20,7 @@ test('no filters broker', t => {
 
   process.chdir(path.resolve(root, '../fixtures/server'));
   const serverPort = port();
-  const server = app.server({ port: serverPort });
+  const server = app.main({ port: serverPort });
 
   process.chdir(path.resolve(root, '../fixtures/client'));
   process.env.SECRET = 'secret';
@@ -29,12 +29,11 @@ test('no filters broker', t => {
   process.env.BROKER_ID = '12345';
   // invalidate the config require
   delete require.cache[require.resolve(__dirname + '/../../lib/config.js')];
-  const client = app.client({ port: port() });
+  const client = app.main({ port: port() });
 
   // wait for the client to successfully connect to the server and identify itself
   server.io.on('connection', socket => {
     socket.on('identify', id => {
-
       t.plan(2);
 
       t.test('successfully broker with no filter should reject', t => {
@@ -47,13 +46,13 @@ test('no filters broker', t => {
       });
 
       t.test('clean up', t => {
-        server.close();
         client.close();
-        t.ok('sockets closed');
-        t.end();
+        setTimeout(() => {
+          server.close();
+          t.ok('sockets closed');
+          t.end();
+        }, 100);
       });
-
-
     });
   });
 
diff --git a/test/unit/filters.test.js b/test/unit/filters.test.js
index fc2a55e62..8e51033ae 100644
--- a/test/unit/filters.test.js
+++ b/test/unit/filters.test.js
@@ -2,7 +2,7 @@ const tap = require('tap').test;
 const Filters = require('../../lib/filters');
 
 tap('filter on body', t => {
-  const filter = Filters(__dirname + '/../fixtures/relay.json');
+  const filter = Filters(require(__dirname + '/../fixtures/relay.json'));
 
   t.plan(8);
   t.ok('filters loaded');
diff --git a/test/utils.js b/test/utils.js
index bdd13dba7..733fc0f02 100644
--- a/test/utils.js
+++ b/test/utils.js
@@ -27,11 +27,16 @@ localServer.all('*', (req, res) => {
 
 
 module.exports = (tap) => {
-  tap.tearDown(() => server.close());
+  tap.tearDown(() => {
+    server.close();
+  });
 
   return {
     localPort,
     port,
     server,
+    resetConfig: () => {
+      delete require.cache[require.resolve(__dirname + '/../lib/config.js')];
+    }
   };
 };