Repository: OWASP/threat-dragon
Pull request #107 (2021-05-03 17:22:38)

Chore/server es6 refactor config
**Summary**
Refactor the td.server/config to use ES6 syntax 

**Description for the changelog**
chore: Refactor td.server/config to use ES6 syntax

**Other info**
All config files are now using es6 syntax.  Improved test coverage a bit as well.



Patch:
diff --git a/td.server/package-lock.json b/td.server/package-lock.json
index 3951cc27..b59e1975 100644
--- a/td.server/package-lock.json
+++ b/td.server/package-lock.json
@@ -9,7 +9,6 @@
       "version": "1.3.1",
       "license": "Apache-2.0",
       "dependencies": {
-        "body-parser": "^1.19.0",
         "bunyan": "1.8.14",
         "connect-azuretables": "1.0.20",
         "connect-ensure-login": "0.1.1",
diff --git a/td.server/package.json b/td.server/package.json
index 65bb2cae..d4d62b33 100644
--- a/td.server/package.json
+++ b/td.server/package.json
@@ -24,7 +24,6 @@
     "url": "git://github.com/owasp/threat-dragon.git"
   },
   "dependencies": {
-    "body-parser": "^1.19.0",
     "bunyan": "1.8.14",
     "connect-azuretables": "1.0.20",
     "connect-ensure-login": "0.1.1",
diff --git a/td.server/src/app.js b/td.server/src/app.js
index bfaac55c..05716940 100644
--- a/td.server/src/app.js
+++ b/td.server/src/app.js
@@ -4,6 +4,12 @@ var path = require('path');
 
 var env = require('./env/Env.js');
 var expressHelper = require('./helpers/express.helper.js');
+var loggers = require('./config/loggers.config.js');
+var parsers = require('./config/parsers.config.js');
+var passport = require('./config/passport.config.js');
+var routes = require('./config/routes.config.js');
+var securityHeaders = require('./config/securityheaders.config.js');
+var session = require('./config/session.config.js');
 
 var upDir = '..' + path.sep;
 var siteDir = path.join(__dirname, upDir, upDir, 'dist');
@@ -22,25 +28,25 @@ function create() {
         app.use('/public', express.static(siteDir));
 
         //security headers
-        require('./config/securityheaders.config').config(app);
+        securityHeaders.default.config(app);
 
         //sessions
-        require('./config/session.config').config(app);
+        session.default.config(app);
 
         //passport
-        require('./config/passport.config').config(app);
+        passport.default.config(app);
 
         //favicon
         app.use(expressHelper.default.getFaviconMiddleware(path.join(siteDir, 'favicon.ico')));
 
         //logging
-        require('./config/loggers.config').config(app);
+        loggers.default.configLoggers(app);
 
         //parsers
-        require('./config/parsers.config').config(app);
+        parsers.default.config(app);
 
         //routes
-        require('./config/routes.config').config(app);
+        routes.default.config(app);
 
         bunyan.createLogger({ name: 'threatdragon', level: 'info' }).info('owasp threat dragon application started up');
 
diff --git a/td.server/src/config/azuretablesession.config.js b/td.server/src/config/azuretablesession.config.js
new file mode 100644
index 00000000..89bc82ca
--- /dev/null
+++ b/td.server/src/config/azuretablesession.config.js
@@ -0,0 +1,7 @@
+import AzureTablesConnect from 'connect-azuretables';
+
+const config = (session, logger) => AzureTablesConnect(session).create({ errorLogger: logger.error.bind(logger) });
+
+export default {
+    config
+};
diff --git a/td.server/src/config/loggers.config.js b/td.server/src/config/loggers.config.js
index 26ddefc3..21b56942 100644
--- a/td.server/src/config/loggers.config.js
+++ b/td.server/src/config/loggers.config.js
@@ -1,6 +1,7 @@
-'use strict';
+import bunyan from 'bunyan';
+import expressBunyanLogger from 'express-bunyan-logger';
 
-var excludes = [
+const excludes = [
     "req-headers",
     "res-headers",
     "res",
@@ -14,17 +15,15 @@ var excludes = [
     "http-version"
 ];
 
-var bunyanOptions = {name: 'threatdragon', level: 'info', excludes: excludes};
+const bunyanOptions = {name: 'threatdragon', level: 'info', excludes: excludes};
 
-function configLoggers(app) {   
-    app.use(require('express-bunyan-logger')(bunyanOptions));
-}
+const configLoggers = (app) => {   
+    app.use(expressBunyanLogger(bunyanOptions));
+};
 
-var logger = require('bunyan').createLogger(bunyanOptions);
+export const logger = bunyan.createLogger(bunyanOptions);
 
-var loggers = {
-    config: configLoggers,
-    logger: logger   
+export default {
+    configLoggers,
+    logger
 };
-
-module.exports = loggers;
diff --git a/td.server/src/config/parsers.config.js b/td.server/src/config/parsers.config.js
index 41870865..24270475 100644
--- a/td.server/src/config/parsers.config.js
+++ b/td.server/src/config/parsers.config.js
@@ -1,18 +1,12 @@
-'use strict';
+import cookieParser from 'cookie-parser';
+import express from 'express';
 
-var cookieParser = require('cookie-parser');
-var bodyParser = require('body-parser');
-
-var parsers = function (app) {
-   
-    app.use(bodyParser.json());
-    app.use(bodyParser.urlencoded({ extended: false }));
-    app.use(cookieParser());  
-    
+const config = (app) => {
+    app.use(express.json());
+    app.use(express.urlencoded({ extended: true }));
+    app.use(cookieParser());
 };
 
-var exports = {
-    config: parsers
+export default {
+    config
 };
-
-module.exports = exports;
diff --git a/td.server/src/config/passport.config.js b/td.server/src/config/passport.config.js
index bd87802f..3149e830 100644
--- a/td.server/src/config/passport.config.js
+++ b/td.server/src/config/passport.config.js
@@ -1,59 +1,65 @@
-'use strict';
+import passport from 'passport';
 
-var passport = require('passport');
-
-var env = require('../env/Env.js');
-var GithubStrategy = require('passport-github').Strategy;
-var passportHelper = require('../helpers/passport.helper.js');
+import env from '../env/Env.js';
+import { Strategy as GithubStrategy } from 'passport-github';
+import passportHelper from '../helpers/passport.helper.js';
 
 /**
- * A fake strategy only used for testing, see comments below
- * @param {object} cfg
+ * A fake strategy only used for testing
  */
-function TestingStrategy(cfg) {
-    this.cfg = cfg;
+class TestingStrategy {
+
+    /**
+     * @constructor
+     * @param {object} cfg
+     */
+    constructor(cfg) {
+        this.cfg = cfg;
+    }
 }
 
 /**
- * Here for testing only.  If someone has a better way, please
- * do so.  I couldn't find a way of mocking a constructor function
- * due to language constraints
- * https://github.com/sinonjs/sinon/issues/1892
+ * Here for testing only: If there's a better way, please open a PR
+ * I couldn't find a reliable way of mocking a constructor function
+ * due to language constraints: https://github.com/sinonjs/sinon/issues/1892
+ * @returns {Class}
  */
-function getStrategy() {
-    return env.default.get().config.IS_TEST === 'true' ? TestingStrategy : GithubStrategy;
-}
+const getStrategy = () => (env.get().config.IS_TEST === 'true' ? TestingStrategy : GithubStrategy);
+
+/**
+ * The passport strategy callback
+ * @param {string} accessToken
+ * @param {string} refreshToken
+ * @param {object} profile
+ * @param {Function} done
+ * @returns {Function}
+ */
+const strategyCallback = (accessToken, refreshToken, profile, done) => done(null, {profile: profile, accessToken: accessToken});
 
-function passportConfig(app) {
-    
+/**
+ * Configures the application to use Passport based on the environment
+ * @param {object} app
+ */
+const config = (app) => {
     app.use(passport.initialize());
     app.use(passport.session());
-    
-    var scope = env.default.get().config.GITHUB_SCOPE || 'public_repo';
-
-    var Strategy = getStrategy();
-    //github sigin
-    passport.use(new Strategy({
-        clientID: env.default.get().config.GITHUB_CLIENT_ID,
-        clientSecret: env.default.get().config.GITHUB_CLIENT_SECRET,
+
+    const githubScope = env.get().config.GITHUB_SCOPE || 'public_repo';
+
+    const Strategy = getStrategy();
+    const githubConfig = {
+        clientID: env.get().config.GITHUB_CLIENT_ID,
+        clientSecret: env.get().config.GITHUB_CLIENT_SECRET,
         failureRedirect: 'login/github',
-        scope: [ scope ]
-    },
-    function(accessToken, refreshToken, profile, done) {
-        return done(null, {profile: profile, accessToken: accessToken});
-    }));
-
-    //serialisation/deserialisation of users
-    //session contains sensitive info like access tokens so encrypt it before storage
-    
-    //encrypt is async to avoid blocking when generating random iv
-    passport.serializeUser(passportHelper.default.serializeUser);
-    passport.deserializeUser(passportHelper.default.deserializeUser);
-}
+        scope: [ githubScope ]
+    };
 
-var exports = {
-    config: passportConfig,
-    TestingStrategy: TestingStrategy
+    passport.use(new Strategy(githubConfig, strategyCallback));
+    passport.serializeUser(passportHelper.serializeUser);
+    passport.deserializeUser(passportHelper.deserializeUser);
 };
 
-module.exports = exports;
+export default {
+    config,
+    TestingStrategy
+};
diff --git a/td.server/src/config/routes.config.js b/td.server/src/config/routes.config.js
index e1f1c445..71e21f40 100644
--- a/td.server/src/config/routes.config.js
+++ b/td.server/src/config/routes.config.js
@@ -1,46 +1,37 @@
-﻿'use strict';
-
-var express = require('express');
-var csrf = require('csurf');
-var home = require('../controllers/homecontroller');
-var github = require('../controllers/githublogincontroller');
-var threatmodel = require('../controllers/threatmodelcontroller');
-
-function addRoutes(app) {
-    var router = express.Router();
-    
-    //anti csrf
-    var csrfProtection = csrf();
-    
-    //main application entry point
-    router.get('/', csrfProtection, home.ensureLoggedIn, home.index);
-
-    router.get('/healthz', function(req, res){ res.send('true');});
-    
-    //login/out
-    router.get('/login', csrfProtection, home.login);
-    router.get('/logoutform', csrfProtection, home.logoutform);
-    router.post('/logout', csrfProtection, home.logout);
-
-    //github sign in
-    router.post('/login', csrfProtection, github.doLogin);
-    router.get('/login/github', github.doLogin);
-    router.get('/oauth/github', github.doLogin, github.completeLogin);
-    
-    //threat models
-    router.get('/threatmodel/repos', home.ensureLoggedIn, threatmodel.repos);
-    router.get('/threatmodel/:organisation/:repo/branches', home.ensureLoggedIn, threatmodel.branches);
-    router.get('/threatmodel/:organisation/:repo/:branch/models', home.ensureLoggedIn, threatmodel.models);
-    router.get('/threatmodel/:organisation/:repo/:branch/:model/data', home.ensureLoggedIn, threatmodel.model);
-    router.delete('/threatmodel/:organisation/:repo/:branch/:model', csrfProtection, home.ensureLoggedIn, threatmodel.deleteModel);
-    router.put('/threatmodel/:organisation/:repo/:branch/:model/create', csrfProtection, home.ensureLoggedIn, threatmodel.create);
-    router.put('/threatmodel/:organisation/:repo/:branch/:model/update', csrfProtection, home.ensureLoggedIn, threatmodel.update);
-    
-    app.use('/', router);
-}
+﻿import csrf from 'csurf';
+import express from 'express';
+
+import githubController from '../controllers/githublogincontroller.js';
+import homeController from '../controllers/homecontroller.js';
+import threatmodelController from '../controllers/threatmodelcontroller.js';
+
+const config = (app) => {
+    const router = express.Router();
+
+    const csrfProtection = csrf();
+
+    router.get('/', csrfProtection, homeController.ensureLoggedIn, homeController.index);
+    router.get('/healthz', (req, res) => res.send('true'));
 
-var exports = {
-    config: addRoutes
+    router.get('/login', csrfProtection, homeController.login);
+    router.get('/logoutform', csrfProtection, homeController.logoutform);
+    router.post('/logout', csrfProtection, homeController.logout);
+
+    router.post('/login', csrfProtection, githubController.doLogin);
+    router.get('/login/github', githubController.doLogin);
+    router.get('/oauth/github', githubController.doLogin, githubController.completeLogin);
+
+    router.get('/threatmodel/repos', homeController.ensureLoggedIn, threatmodelController.repos);
+    router.get('/threatmodel/:organisation/:repo/branches', homeController.ensureLoggedIn, threatmodelController.branches);
+    router.get('/threatmodel/:organisation/:repo/:branch/models', homeController.ensureLoggedIn, threatmodelController.models);
+    router.get('/threatmodel/:organisation/:repo/:branch/:model/data', homeController.ensureLoggedIn, threatmodelController.model);
+    router.delete('/threatmodel/:organisation/:repo/:branch/:model', csrfProtection, homeController.ensureLoggedIn, threatmodelController.deleteModel);
+    router.put('/threatmodel/:organisation/:repo/:branch/:model/create', csrfProtection, homeController.ensureLoggedIn, threatmodelController.create);
+    router.put('/threatmodel/:organisation/:repo/:branch/:model/update', csrfProtection, homeController.ensureLoggedIn, threatmodelController.update);
+
+    app.use('/', router);
 };
 
-module.exports = exports;
+export default {
+    config
+};
diff --git a/td.server/src/config/securityheaders.config.js b/td.server/src/config/securityheaders.config.js
index f935c223..164adefd 100644
--- a/td.server/src/config/securityheaders.config.js
+++ b/td.server/src/config/securityheaders.config.js
@@ -1,11 +1,9 @@
-'use strict';
+import helmet from 'helmet';
 
-var helmet = require('helmet');
-
-var securityHeaders = function (app, forceSecure) {
-    
+const config = (app, forceSecure) => {
     app.set('x-powered-by', false);
-    var ninetyDaysInSeconds = 7776000;
+    const ninetyDaysInSeconds = 7776000;
+    // Is forceSecure ever used?
     app.use(helmet.hsts({ maxAge: ninetyDaysInSeconds, force: forceSecure, includeSubDomains: false }));
     app.use(helmet.frameguard({action: 'deny'}));
     app.use(helmet.hidePoweredBy());
@@ -27,8 +25,6 @@ var securityHeaders = function (app, forceSecure) {
     }));
 };
 
-var exports = {
-    config: securityHeaders
+export default {
+    config
 };
-
-module.exports = exports;
diff --git a/td.server/src/config/session.config.js b/td.server/src/config/session.config.js
index 2f2747c6..21a87f29 100644
--- a/td.server/src/config/session.config.js
+++ b/td.server/src/config/session.config.js
@@ -1,41 +1,37 @@
-'use strict';
+import session from 'express-session';
 
-var session = require('express-session');
-var AzureTablesStoreFactory = require('connect-azuretables')(session);
+import azureTableSession from './azuretablesession.config.js';
+import env from '../env/Env.js';
+import { logger } from './loggers.config.js';
 
-var env = require('../env/Env.js');
-
-function configSessions(app) {
-
-    var cookieOptions = { maxAge: 3600000 };
-    var logger = require('../config/loggers.config').logger;
-
-    if (env.default.get().config.NODE_ENV && env.default.get().config.NODE_ENV != 'development') {
-        cookieOptions.secure = true;
-    } else {
-        logger.error({ security: true }, 'secure session cookie flag was false - should only happen in dev environments');
-    }
+const config = (app) => {
+    const cookieOptions = {
+        maxAge: 3600000,
+        secure: env.get().config.NODE_ENV !== 'development'
+    };
 
-    var sessionOptions = {
-        secret: env.default.get().config.SESSION_SIGNING_KEY,
+    const sessionOptions = {
+        secret: env.get().config.SESSION_SIGNING_KEY,
         resave: false,
         saveUninitialized: false,
         rolling: true,
         cookie: cookieOptions
     };
 
-    if (env.default.get().config.SESSION_STORE != 'local') {
-        sessionOptions.store = AzureTablesStoreFactory.create({ errorLogger: logger.error.bind(logger) });
+    if (!cookieOptions.secure) {
+        logger.error({ security: true }, 'secure session cookie flag was false - should only happen in dev environments');
+    }
+
+    if (env.get().config.SESSION_STORE !== 'local') {
+        sessionOptions.store = azureTableSession.config(session, logger);
     } else {
         //use in-memory session store
         logger.error({ security: true }, 'local session store used - should only happen in dev environments');
     }
 
     app.use(session(sessionOptions));
-}
-
-var exports = {
-    config: configSessions
 };
 
-module.exports = exports;
+export default {
+    config
+};
diff --git a/td.server/src/controllers/homecontroller.js b/td.server/src/controllers/homecontroller.js
index 49999999..2f334d79 100644
--- a/td.server/src/controllers/homecontroller.js
+++ b/td.server/src/controllers/homecontroller.js
@@ -12,7 +12,7 @@ homeController.index = function (req, res) {
     if(env.default.get().config.NODE_ENV != 'development' && env.default.get().config.NODE_ENV) {
         cookieOptions.secure = true;
     } else {
-        require('../config/loggers.config').logger.error({security: true}, 'secure anti-XSRF cookie flag was false - should only happen in dev environments');
+        require('../config/loggers.config').default.logger.error({security: true}, 'secure anti-XSRF cookie flag was false - should only happen in dev environments');
     }
     
     res.cookie('XSRF-TOKEN', req.csrfToken(), cookieOptions);
diff --git a/td.server/src/helpers/encryption.helper.js b/td.server/src/helpers/encryption.helper.js
index 214d5e46..7f33a6b3 100644
--- a/td.server/src/helpers/encryption.helper.js
+++ b/td.server/src/helpers/encryption.helper.js
@@ -22,7 +22,7 @@ function getPrimaryKey() {
 
     if (!primaryKey) {
         var message = 'missing primary session encryption key';
-        require('../config/loggers.config').logger.fatal(message);
+        require('../config/loggers.config').default.logger.fatal(message);
         throw new Error(message);
     }
     
@@ -36,7 +36,7 @@ function getKeyById(id) {
 
     if (!key) {
         var message = 'missing session encryption key id:  ' + id;
-        require('../config/loggers.config').logger.error(message);
+        require('../config/loggers.config').default.logger.error(message);
         throw new Error(message);
     }
     


Test patch:
diff --git a/td.server/test/app_spec.js b/td.server/test/app_spec.js
index 9508cbb3..40c26d74 100644
--- a/td.server/test/app_spec.js
+++ b/td.server/test/app_spec.js
@@ -36,7 +36,7 @@ describe('app tests', () => {
         sinon.stub(sessionConfig, 'config');
         sinon.stub(expressHelper, 'getFaviconMiddleware');
         sinon.stub(passportConfig, 'config');
-        sinon.stub(loggersConfig, 'config');
+        sinon.stub(loggersConfig, 'configLoggers');
         sinon.stub(parsersConfig, 'config');
         sinon.stub(routesConfig, 'config');
     });
@@ -89,7 +89,7 @@ describe('app tests', () => {
         });
 
         it('uses the loggers config', () => {
-            expect(loggersConfig.config).to.have.been.calledOnce;
+            expect(loggersConfig.configLoggers).to.have.been.calledOnce;
         });
 
         it('uses the parsers config', () => {
diff --git a/td.server/test/config/loggers.config_spec.js b/td.server/test/config/loggers.config_spec.js
index 4291df57..5cf92bdf 100644
--- a/td.server/test/config/loggers.config_spec.js
+++ b/td.server/test/config/loggers.config_spec.js
@@ -2,17 +2,17 @@ import { expect } from 'chai';
 import request from 'supertest';
 import sinon from 'sinon';
 
+import loggers from '../../src/config/loggers.config.js';
+
 describe('loggers config tests', () => {
     let app;
-    let loggers;
     
     beforeEach(() => {
         app = require('express')();
-        loggers = require('../../src/config/loggers.config');
     });
     
     it('should set the logger on requests', (done) => {
-        loggers.config(app);
+        loggers.configLoggers(app);
         app.get('/test', (req, res) => {
             expect(req.log).not.to.be.undefined;
             res.status(200).send('result');
diff --git a/td.server/test/config/parsers.config_spec.js b/td.server/test/config/parsers.config_spec.js
index 52cafd1d..4f51de19 100644
--- a/td.server/test/config/parsers.config_spec.js
+++ b/td.server/test/config/parsers.config_spec.js
@@ -2,12 +2,14 @@ import { expect } from 'chai';
 import express from 'express';
 import request from 'supertest';
 
+import parsers from '../../src/config/parsers.config.js';
+
 describe('request parser tests', () => {
     let app;
     
     beforeEach(() => {
         app = express();
-        require('../../src/config/parsers.config').config(app);
+        parsers.config(app);
     });
     
     it('should parse the json in the request body', (done) => {
@@ -38,15 +40,15 @@ describe('request parser tests', () => {
         const body = {id: id, collection: collection, nested: nested};
         
         app.post('/', function(req, res) {
-            expect(req.body["collection[0]"]).to.eq(collection[0]);
-            expect(req.body["collection[1]"]).to.eq(collection[1]);
-            expect(req.body["collection[2]"]).to.eq(collection[2]);
-            expect(req.body["nested[n1]"]).to.eq(nested.n1);
-            expect(req.body["nested[n2]"]).to.eq(nested.n2);
+            expect(req.body.collection[0]).to.eq(collection[0]);
+            expect(req.body.collection[1]).to.eq(collection[1]);
+            expect(req.body.collection[2]).to.eq(collection[2]);
+            expect(req.body.nested.n1).to.eq(nested.n1);
+            expect(req.body.nested.n2).to.eq(nested.n2);
             res.status(200);
             res.send('result');
         });
-        
+
         request(app)
             .post('/')
             .type('form')
diff --git a/td.server/test/config/session.config_spec.js b/td.server/test/config/session.config_spec.js
index e771aa7e..1b7f9e95 100644
--- a/td.server/test/config/session.config_spec.js
+++ b/td.server/test/config/session.config_spec.js
@@ -1,7 +1,8 @@
-import AzureTablesStoreFactory from 'connect-azuretables';
 import { expect } from 'chai';
+import session from 'express-session';
 import sinon from 'sinon';
 
+import azureTableSession from '../../src/config/azuretablesession.config.js';
 import env from '../../src/env/Env.js';
 import loggers from '../../src/config/loggers.config.js';
 import sessionConfig from '../../src/config/session.config.js';
@@ -25,7 +26,8 @@ describe('session config tests', () => {
         const mockEnv = {
             config: {
                 SESSION_SIGNING_KEY: sessionSigningKey,
-                SESSION_STORE: 'local'
+                SESSION_STORE: 'local',
+                NODE_ENV: 'development'
             }
         };
         beforeEach(() => {
@@ -62,7 +64,9 @@ describe('session config tests', () => {
         };
         beforeEach(() => {
             sinon.stub(env, 'get').returns(mockEnv);
-            sinon.spy(AzureTablesStoreFactory);
+            sinon.stub(loggers.logger, 'info');
+            sinon.stub(azureTableSession, 'config');
+            sessionConfig.config(mockApp);
         });
 
         afterEach(() => {
@@ -74,7 +78,7 @@ describe('session config tests', () => {
         });
 
         it('sets the session store to azure table storage', () => {
-            expect(loggers.logger.error).not.to.have.been.called;
+            expect(azureTableSession.config).to.have.been.calledOnce;
         });
     });
 });