tests: always use real bcrypt lib (#962)

Instead of mocking bcrypt in some test scenarios, decrease the cost factor to
the fastest possible value.

* remove dependency injection of `bcrypt`
* remove `mock-bcrypt.js`
* remove multiple paths to `password.verify()`/`verifyPassword()` and `password.hash()`/`hashPassword()`
* cut CI build times from ~8.5 mins to ~2.5 mins

.circleci/config.yml

@@ -14,5 +14,5 @@ jobs:
       - run: make check-file-headers
       - run: npm ci --legacy-peer-deps
       - run: node lib/bin/create-docker-databases.js
-      - run: make test-full
+      - run: make test
 

.github/PULL_REQUEST_TEMPLATE.md

@@ -17,5 +17,5 @@ https://github.com/getodk/central-backend/blob/master/CONTRIBUTING.md
 
 #### Before submitting this PR, please make sure you have:
 
-- [ ] run `make test-full` and confirmed all checks still pass OR confirm CircleCI build passes
+- [ ] run `make test` and confirmed all checks still pass OR confirm CircleCI build passes
 - [ ] verified that any code from external sources are properly credited in comments or that everything is internally sourced

.github/workflows/standard-suite.yml

@@ -30,4 +30,4 @@ jobs:
         cache: 'npm'
     - run: npm ci --legacy-peer-deps
     - run: node lib/bin/create-docker-databases.js
-    - run: make test-full
+    - run: make test

Makefile

@@ -60,23 +60,19 @@ debug: base
 
 .PHONY: test
 test: lint
-	BCRYPT=no npx mocha --recursive --exit
-
-.PHONY: test-full
-test-full: lint
-	npx mocha --recursive --exit
+	BCRYPT=insecure npx mocha --recursive --exit
 
 .PHONY: test-fast
 test-fast: node_version
 	BCRYPT=no npx mocha --recursive --exit --fgrep @slow --invert
 
 .PHONY: test-integration
 test-integration: node_version
-	BCRYPT=no npx mocha --recursive test/integration --exit
+	BCRYPT=insecure npx mocha --recursive test/integration --exit
 
 .PHONY: test-unit
 test-unit: node_version
-	npx mocha --recursive test/unit --exit
+	BCRYPT=insecure npx mocha --recursive test/unit --exit
 
 .PHONY: test-coverage
 test-coverage: node_version

lib/bin/run-server.js

@@ -34,7 +34,6 @@ Error.stackTrackLimit = 20;
 
 // get an xlsform client and a password module.
 const xlsform = require('../external/xlsform').init(config.get('default.xlsform'));
-const bcrypt = require('../util/crypto').password(require('bcrypt'));
 
 // get an Enketo client
 const enketo = require('../external/enketo').init(config.get('default.enketo'));
@@ -45,7 +44,7 @@ const enketo = require('../external/enketo').init(config.get('default.enketo'));
 
 // initialize our container, then generate an http service out of it.
 const container = require('../model/container')
-  .withDefaults({ db, mail, env, Sentry, bcrypt, xlsform, enketo });
+  .withDefaults({ db, mail, env, Sentry, xlsform, enketo });
 const service = require('../http/service')(container);
 
 // insert the graceful exit middleware.

lib/http/preprocessors.js

@@ -7,6 +7,7 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 
+const { verifyPassword } = require('../util/crypto');
 const { isBlank, isPresent, noop, without } = require('../util/util');
 const { isTrue } = require('../util/http');
 const oidc = require('../util/oidc');
@@ -27,7 +28,7 @@ const emptyAuthInjector = ({ Auth }, context) => context.with({ auth: Auth.by(nu
 // in conjunction with this function!
 //
 // TODO?: repetitive, but deduping it makes it even harder to understand.
-const authHandler = ({ Sessions, Users, Auth, bcrypt }, context) => {
+const authHandler = ({ Sessions, Users, Auth }, context) => {
   const authBySessionToken = (token, onFailure = noop) => Sessions.getByBearerToken(token)
     .then((session) => {
       if (!session.isDefined()) return onFailure();
@@ -87,7 +88,7 @@ const authHandler = ({ Sessions, Users, Auth, bcrypt }, context) => {
     // TODO: email existence timing attack on whether bcrypt runs or not.
     return Users.getByEmail(email)
       .then(getOrReject(Problem.user.authenticationFailed()))
-      .then((user) => bcrypt.verify(password, user.password)
+      .then((user) => verifyPassword(password, user.password)
         .then((verified) => {
           if (verified === true)
             return context.with({ auth: Auth.by(user.actor) });

lib/model/query/users.js

@@ -10,6 +10,7 @@
 const { sql } = require('slonik');
 const { map } = require('ramda');
 const { Actor, User } = require('../frames');
+const { hashPassword } = require('../../util/crypto');
 const { unjoiner, page, equals, QueryOptions } = require('../../util/db');
 const { reject } = require('../../util/promise');
 const Problem = require('../../util/problem');
@@ -32,10 +33,10 @@ const update = (user, data) => ({ run, one }) => {
 
 update.audit = (user, data) => (log) => log('user.update', user.actor, { data: data.with(data.actor) });
 
-const updatePassword = (user, cleartext) => ({ run, bcrypt }) =>
+const updatePassword = (user, cleartext) => ({ run }) =>
   (cleartext.length < 10
     ? reject(Problem.user.passwordTooShort())
-    : bcrypt.hash(cleartext))
+    : hashPassword(cleartext))
     .then((hash) => run(sql`update users set password=${hash} where "actorId"=${user.actor.id}`));
 updatePassword.audit = (user) => (log) => log('user.update', user.actor, { data: { password: true } });
 

lib/resources/sessions.js

@@ -7,6 +7,7 @@
 // including this file, may be copied, modified, propagated, or distributed
 // except according to the terms contained in the LICENSE file.
 
+const { verifyPassword } = require('../util/crypto');
 const Problem = require('../util/problem');
 const { isBlank, noargs } = require('../util/util');
 const { getOrReject, rejectIf } = require('../util/promise');
@@ -17,9 +18,9 @@ const oidc = require('../util/oidc');
 module.exports = (service, endpoint) => {
 
   if (!oidc.isEnabled()) {
-    service.post('/sessions', endpoint(({ Audits, Users, Sessions, bcrypt }, { body, headers }) => {
+    service.post('/sessions', endpoint(({ Audits, Users, Sessions }, { body, headers }) => {
       // TODO if we're planning to offer multiple authN methods, we should be looking for
-      // any calls to bcrypt.verify(), and blocking them if that authN method is not
+      // any calls to verifyPassword(), and blocking them if that authN method is not
       // appropriate for the current user.
       //
       // It may be useful to re-use the sessions resources for other authN methods.
@@ -31,7 +32,7 @@ module.exports = (service, endpoint) => {
 
       return Users.getByEmail(email)
         .then(getOrReject(Problem.user.authenticationFailed()))
-        .then((user) => bcrypt.verify(password, user.password)
+        .then((user) => verifyPassword(password, user.password)
           .then(rejectIf(
             (verified) => (verified !== true),
             noargs(Problem.user.authenticationFailed)

lib/resources/users.js

@@ -9,6 +9,7 @@
 
 const { always } = require('ramda');
 const { User } = require('../model/frames');
+const { verifyPassword } = require('../util/crypto');
 const { success, isTrue } = require('../util/http');
 const Option = require('../util/option');
 const Problem = require('../util/problem');
@@ -90,10 +91,10 @@ module.exports = (service, endpoint) => {
 
     // TODO: infosec debate around 404 vs 403 if insufficient privs but record DNE.
     // TODO: exact endpoint naming.
-    service.put('/users/:id/password', endpoint(async ({ Sessions, Users, mail, bcrypt }, { params, body, auth }) => {
+    service.put('/users/:id/password', endpoint(async ({ Sessions, Users, mail }, { params, body, auth }) => {
       const user = await Users.getByActorId(params.id).then(getOrNotFound);
       await auth.canOrReject('user.update', user.actor);
-      const verified = await bcrypt.verify(body.old, user.password);
+      const verified = await verifyPassword(body.old, user.password);
       if (verified !== true) return Problem.user.authenticationFailed();
       await Promise.all([
         Users.updatePassword(user, body.new),

lib/task/task.js

@@ -26,7 +26,6 @@ const { serialize } = require('../util/http');
 
 
 // initialize modules we need to put in the container:
-const bcrypt = require('../util/crypto').password(require('bcrypt'));
 const env = config.get('default.env');
 const { mailer } = require('../external/mail');
 const mail = mailer(mergeRight(config.get('default.email'), { env }));
@@ -47,7 +46,7 @@ const task = {
   // not thread-safe! but we don't have threads..
   withContainer: (taskdef) => (...args) => {
     const needsContainer = (task._container == null);
-    if (needsContainer) task._container = container.withDefaults({ db: slonikPool(config.get('default.database')), bcrypt, env, mail, task: true, odkAnalytics });
+    if (needsContainer) task._container = container.withDefaults({ db: slonikPool(config.get('default.database')), env, mail, task: true, odkAnalytics });
 
     const result = taskdef(task._container)(...args);
 

lib/util/crypto.js

@@ -10,6 +10,7 @@
 // Various useful cryptography functions, for doing things like hashing passwords,
 // generating random tokens, and encrypting/decrypting data.
 
+const bcrypt = require('bcrypt');
 const digest = require('digest-stream');
 const { createHash, randomBytes, generateKeyPair, pbkdf2, createPrivateKey, createPublicKey, createCipheriv, createDecipheriv, publicEncrypt, privateDecrypt } = require('crypto');
 const { RSA_NO_PADDING } = require('crypto').constants;
@@ -25,13 +26,17 @@ const { isBlank } = require('./util');
 ////////////////////////////////////////////////////////////////////////////////
 // PASSWORD/AUTH UTIL
 
+// In tests, allow an insecure cost-factor for bcrypt.  In fact the minimum salt
+// is 4, but this is only documented in the bcrypt lib's issue tracker.
+// See: https://github.com/kelektiv/node.bcrypt.js/issues/870
+const BCRYPT_COST_FACTOR = process.env.BCRYPT === 'insecure' ? 1 : 12;
+
 // These functions call into bcrypt to hash or verify passwords.
-const hashPassword = (bcrypt) => (plain) =>
-  (isBlank(plain) ? resolve(null) : bcrypt.hash(plain, 12));
-const verifyPassword = (bcrypt) => (plain, hash) => ((isBlank(plain) || (isBlank(hash)))
+const hashPassword = (plain) =>
+  (isBlank(plain) ? resolve(null) : bcrypt.hash(plain, BCRYPT_COST_FACTOR));
+const verifyPassword = (plain, hash) => ((isBlank(plain) || (isBlank(hash)))
   ? resolve(false)
   : bcrypt.compare(plain, hash));
-const password = (bcrypt) => ({ hash: hashPassword(bcrypt), verify: verifyPassword(bcrypt) });
 
 // Returns a cryptographically random base64 string of a given length.
 const generateToken = (bytes = 48) => randomBytes(bytes).toString('base64')
@@ -302,7 +307,7 @@ const submissionDecryptor = (keys, passphrases) =>
 
 
 module.exports = {
-  hashPassword, verifyPassword, password, generateToken,
+  hashPassword, verifyPassword, generateToken,
   shasum, sha256sum, md5sum, digestWith,
   getPrivate, generateManagedKey, generateVersionSuffix,
   stripPemEnvelope,

test/integration/fixtures/01-users.js

@@ -1,8 +1,9 @@
 const appRoot = require('app-root-path');
 const { User, Actor, Project } = require(appRoot + '/lib/model/frames');
+const { hashPassword } = require(appRoot + '/lib/util/crypto');
 const { mapSequential } = require(appRoot + '/test/util/util');
 
-module.exports = ({ Assignments, Users, Projects, bcrypt }) => {
+module.exports = ({ Assignments, Users, Projects }) => {
   const proj = new Project({ name: 'Default Project' });
 
   const users = [
@@ -13,7 +14,7 @@ module.exports = ({ Assignments, Users, Projects, bcrypt }) => {
 
   // hash the passwords, create our three test users, then add grant Alice and Bob their rights.
   const withPasswords = Promise.all(users.map((user) =>
-    bcrypt.hash(user.password).then((password) => user.with({ password }))));
+    hashPassword(user.password).then((password) => user.with({ password }))));
 
   return Projects.create(proj)
     .then(() => withPasswords)

test/integration/setup.js

@@ -33,12 +33,6 @@ const xlsform = require(appRoot + '/test/util/xlsform');
 // set up our sentry mock.
 const Sentry = require(appRoot + '/lib/external/sentry').init();
 
-// set up our bcrypt module; possibly mock or not based on params.
-const _bcrypt = (process.env.BCRYPT === 'no')
-  ? require('../util/bcrypt-mock')
-  : require('bcrypt');
-const bcrypt = require(appRoot + '/lib/util/crypto').password(_bcrypt);
-
 // set up our enketo mock.
 const { reset: resetEnketo, ...enketo } = require(appRoot + '/test/util/enketo');
 // Initialize the mock before other setup that uses the mock, then reset the
@@ -87,7 +81,7 @@ const initialize = async () => {
     await migrator.destroy();
   }
 
-  return withDefaults({ db, bcrypt, context, enketo, env }).transacting(populate);
+  return withDefaults({ db, context, enketo, env }).transacting(populate);
 };
 
 // eslint-disable-next-line func-names, space-before-function-paren
@@ -143,7 +137,7 @@ const augment = (service) => {
 // FINAL TEST WRAPPERS
 
 
-const baseContainer = withDefaults({ db, mail, env, xlsform, bcrypt, enketo, Sentry, odkAnalytics, context });
+const baseContainer = withDefaults({ db, mail, env, xlsform, enketo, Sentry, odkAnalytics, context });
 
 // called to get a service context per request. we do some work to hijack the
 // transaction system so that each test runs in a single transaction that then

test/integration/task/account.js

@@ -1,6 +1,7 @@
 const appRoot = require('app-root-path');
 const should = require('should');
 const { testTask } = require('../setup');
+const { verifyPassword } = require(appRoot + '/lib/util/crypto');
 const { getOrNotFound } = require(appRoot + '/lib/util/promise');
 const { createUser, promoteUser, setUserPassword } = require(appRoot + '/lib/task/account');
 const { User } = require(appRoot + '/lib/model/frames');
@@ -35,18 +36,18 @@ describe('task: accounts', () => {
           should(log.details.data.password).equal(null);
         })));
 
-    it('should set the password if given', testTask(({ Users, bcrypt }) =>
+    it('should set the password if given', testTask(({ Users }) =>
       createUser('[email protected]', 'aoeuidhtns')
         .then(() => Users.getByEmail('[email protected]'))
         .then(getOrNotFound)
-        .then((user) => bcrypt.verify('aoeuidhtns', user.password))
+        .then((user) => verifyPassword('aoeuidhtns', user.password))
         .then((verified) => verified.should.equal(true))));
 
-    it('should not verify a null password', testTask(({ Users, bcrypt }) =>
+    it('should not verify a null password', testTask(({ Users }) =>
       createUser('[email protected]', null)
         .then(() => Users.getByEmail('[email protected]'))
         .then(getOrNotFound)
-        .then((user) => bcrypt.verify(null, user.password))
+        .then((user) => verifyPassword(null, user.password))
         .then((verified) => verified.should.equal(false))));
 
     it('should complain if the password is too short', testTask(() =>
@@ -85,12 +86,12 @@ describe('task: accounts', () => {
   });
 
   describe('setUserPassword', () => {
-    it('should set a user password', testTask(({ Users, bcrypt }) =>
+    it('should set a user password', testTask(({ Users }) =>
       Users.create(User.fromApi({ email: '[email protected]', displayName: 'test user' }))
         .then(() => setUserPassword('[email protected]', 'aoeuidhtns'))
         .then(() => Users.getByEmail('[email protected]'))
         .then(getOrNotFound)
-        .then((user) => bcrypt.verify('aoeuidhtns', user.password))
+        .then((user) => verifyPassword('aoeuidhtns', user.password))
         .then((verified) => verified.should.equal(true))));
 
     it('should complain about a password that is too short', testTask(({ Users }) =>

test/unit/http/preprocessors.js

@@ -6,11 +6,10 @@ const preprocessors = require(appRoot + '/lib/http/preprocessors');
 const { Context } = require(appRoot + '/lib/http/endpoint');
 const { Session, User, Actor } = require(appRoot + '/lib/model/frames');
 const { by } = require(appRoot + '/lib/model/query/auth');
+const { hashPassword } = require(appRoot + '/lib/util/crypto');
 const Problem = require(appRoot + '/lib/util/problem');
 const Option = require(appRoot + '/lib/util/option');
 
-const bcrypt = require(appRoot + '/lib/util/crypto').password(require('bcrypt'));
-
 describe('preprocessors', () => {
   // some mock helpers to simplify testing this module in isolation:
   const Auth = { by: (x) => by(x)({}) };
@@ -115,7 +114,7 @@ describe('preprocessors', () => {
 
       it('should fail the request if the Basic auth credentials are not right', () =>
         Promise.resolve(authHandler(
-          { Auth, Users: mockUsers('[email protected]', 'willnevermatch'), bcrypt },
+          { Auth, Users: mockUsers('[email protected]', 'willnevermatch') },
           new Context(
             createRequest({ headers: {
               Authorization: `Basic ${Buffer.from('[email protected]:alice', 'utf8').toString('base64')}`,
@@ -126,9 +125,9 @@ describe('preprocessors', () => {
         )).should.be.rejectedWith(Problem, { problemCode: 401.2 }));
 
       it('should set the appropriate session if valid Basic auth credentials are given @slow', () =>
-        bcrypt.hash('alice').then((hashed) =>
+        hashPassword('alice').then((hashed) =>
           Promise.resolve(authHandler(
-            { Auth, Users: mockUsers('[email protected]', hashed), bcrypt },
+            { Auth, Users: mockUsers('[email protected]', hashed) },
             new Context(
               createRequest({ headers: {
                 Authorization: `Basic ${Buffer.from('[email protected]:alice', 'utf8').toString('base64')}`,

test/unit/util/crypto.js

@@ -2,38 +2,38 @@ const appRoot = require('app-root-path');
 const { readFileSync } = require('fs');
 const should = require('should');
 const streamTest = require('streamtest').v2;
-const bcrypt = require('bcrypt');
 const crypto = require(appRoot + '/lib/util/crypto');
 
 describe('util/crypto', () => {
-  describe('hashPassword/verifyPassword @slow', () => {
-    const password = crypto.password(bcrypt);
+  describe('hashPassword/verifyPassword', () => {
+    const { hashPassword, verifyPassword } = crypto;
+
     // we do not actually verify the hashing itself, as:
     // 1. it is entirely performed by bcrypt, which has is own tests.
     // 2. bcrypt is intentionally slow, and we would like unit tests to be fast.
 
     it('should always return a Promise', () => {
-      password.hash('').should.be.a.Promise();
-      password.hash('password').should.be.a.Promise();
-      password.verify('password', 'hashhash').should.be.a.Promise();
+      hashPassword('').should.be.a.Promise();
+      hashPassword('password').should.be.a.Promise();
+      hashPassword('password', 'hashhash').should.be.a.Promise();
     });
 
     it('should return a Promise of null given a blank plaintext', (done) => {
-      password.hash('').then((result) => {
+      hashPassword('').then((result) => {
         should(result).equal(null);
         done();
       });
     });
 
     it('should not attempt to verify empty plaintext', (done) => {
-      password.verify('', '$2a$12$hCRUXz/7Hx2iKPLCduvrWugC5Q/j5e3bX9KvaYvaIvg/uvFYEpzSy').then((result) => {
+      verifyPassword('', '$2a$12$hCRUXz/7Hx2iKPLCduvrWugC5Q/j5e3bX9KvaYvaIvg/uvFYEpzSy').then((result) => {
         result.should.equal(false);
         done();
       });
     });
 
     it('should not attempt to verify empty hash', (done) => {
-      password.verify('password', '').then((result) => {
+      verifyPassword('password', '').then((result) => {
         result.should.equal(false);
         done();
       });