From 193dc713fd2a9be3dddfc8ecead10190538b68fd Mon Sep 17 00:00:00 2001 From: Ryan Kelly Date: Mon, 26 Nov 2018 12:36:48 +1100 Subject: [PATCH] feat(service): Create a nice abstraction for calling backend service APIs --- lib/backendService.js | 188 +++++++++++++++++ lib/customs.js | 251 ++++++++++++----------- lib/error.js | 11 + lib/pushbox.js | 140 ++++++------- lib/routes/devices-and-sessions.js | 6 +- lib/routes/recovery-key.js | 3 +- lib/safe-url.js | 6 +- test/local/backendService.js | 314 +++++++++++++++++++++++++++++ test/local/customs.js | 12 +- test/local/pushbox.js | 204 ++++++++----------- test/local/routes/recovery-keys.js | 6 +- 11 files changed, 813 insertions(+), 328 deletions(-) create mode 100644 lib/backendService.js create mode 100644 test/local/backendService.js diff --git a/lib/backendService.js b/lib/backendService.js new file mode 100644 index 00000000..ed6fdf81 --- /dev/null +++ b/lib/backendService.js @@ -0,0 +1,188 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +/* + * A helper class for safely making calls to a backend service over HTTP. + * + * The `createBackendServiceAPI` function lets you declaratively specify the routes to be + * called on a remote HTTP service, in a style similar to Hapi's own route configuration + * options. It produces an object with friendly async methods that, when called, will take + * care of the following important implementation details for you: + * + * - using a connection pool with sensible configuration + * - guarding against unexpected data in input parameters + * - guarding against unexpected data in responses + * - throwing appropriate errors on service failure + * - logging appropriate diagnostics + * + * Declare the API of the service like this: + * + * const MyExampleService = createBackendServiceAPI(log, config, "name", { + * + * getFoo: { + * method: 'GET', + * path: '/v1/foo/:name', + * validate: { + * params: { + * name: Joi.string().length(20) + * }, + * response: { + * name: Joi.string().length(20), + * barCount: Joi.integer() + * } + * } + * } + * + * setFoo: { + * method: 'POST', + * path: '/v1/foo/:name', + * validate: { + * params: { + * name: Joi.string().length(20) + * }, + * payload: { + * numBars: Joi.integer() + * } + * response: { + * name: Joi.string().length(20), + * barCount: Joi.integer() + * } + * } + * } + * + * }) + * + * And then call it like this: + * + * const example = new MyExampleService("https://example.com/") + * + * let foo = await example.getFoo("test") + * assert.equal(foo.name, "test") + * assert.equal(foo.barCount, 0) + * + * foo = await.example.setFoo("test", { numBars: 12 }) + * assert.equal(foo.barCount, 12) + * + */ + +'use strict' + +const Joi = require('joi') + +const P = require('./promise') +const Pool = require('./pool') +const error = require('./error') + +module.exports = function createBackendServiceAPI(log, config, serviceName, methods) { + + const SafeUrl = require('./safe-url')(log) + + function Service(url, options = {}) { + this._headers = options.headers + this._pool = new Pool(url, options) + } + + Service.prototype.close = function close() { + return this._pool.close() + } + + for (const methodName in methods) { + Service.prototype[methodName] = makeServiceMethod(methodName, methods[methodName]) + } + + return Service + + // Each declared service method gets turned into an async function + // that validates its inputs, makes the HTTP request using the + // connection pool, and validates the response. + + function makeServiceMethod(methodName, opts) { + const path = new SafeUrl(opts.path) + + const validation = opts.validate || {} + const paramsSchema = Joi.compile(validation.params || Joi.object()) + const querySchema = Joi.compile(validation.query || Joi.object()) + const payloadSchema = Joi.compile(validation.payload || Joi.object()) + const responseSchema = Joi.compile(validation.response || Joi.any()) + + let expectedNumArgs = path.params().length + if (validation.query) { + expectedNumArgs += 1 + } + if (validation.payload) { + expectedNumArgs += 1 + } + + const fullMethodName = `${serviceName}.${methodName}` + + // A thin wrapper around Joi.validate(), that logs the error and then + // wraps it in a generic "internal validation error" that can be returned + // to the client. + + function validate(location, value, schema, options) { + return new P((resolve, reject) => { + Joi.validate(value, schema, options, (err, value) => { + if (! err) { + return resolve(value) + } + log.error({ + op: fullMethodName, + error: `${location} schema validation failed`, + message: err.message, + value + }) + reject(error.internalValidationError()) + }) + }) + } + + // A helper to make the request and return the response, or an error. + // This assumes you've done all the hard work of formulating params, body, etc. + + async function sendRequest(pool, method, path, params, query, payload, headers) { + log.trace({ op: fullMethodName, params, query, payload }) + try { + return await pool.request(method, path, params, query, payload, headers) + } catch (err) { + // Re-throw 400-level errors, but wrap 500-level or generic errors + // into a "backend service failure" to propagate to the client. + if (err.errno || (err.statusCode && err.statusCode < 500)) { + throw err + } else { + log.error({ op: `${fullMethodName}.1`, params, query, payload, err }) + throw error.backendServiceFailure(serviceName, methodName) + } + } + } + + // The actual method implementation. + + async function theServiceMethod(...args) { + // Interpret function arguments according to the declared schema. + if (args.length !== expectedNumArgs) { + throw new Error(`${fullMethodName} must be called with ${expectedNumArgs} arguments (${args.length} given)`) + } + let i = 0 + // The leading positional arguments correspond to individual path params, + // in the order they appear in the path template. + let params = {} + for (const param of path.params()) { + params[param] = args[i++] + } + params = await validate('params', params, paramsSchema) + // Next are query params as a dict, if any. + const query = validation.query ? await validate('query', args[i++], querySchema) : {} + // Next is request payload as a dict, if any. + const payload = validation.payload ? await validate('request', args[i++], payloadSchema) : {} + // Unexpected extra fields in the service response should not be a fatal error, + // but we also don't want them polluting our code. So, stripUnknown=true. + const response = await sendRequest(this._pool, opts.method, path, params, query, payload, this._headers) + return await validate('response', response, responseSchema, { stripUnknown: true }) + } + + // Expose the options for introspection by calling code if necessary. + theServiceMethod.opts = opts + return theServiceMethod + } +} diff --git a/lib/customs.js b/lib/customs.js index 054e6b81..6d8f6226 100644 --- a/lib/customs.js +++ b/lib/customs.js @@ -4,17 +4,100 @@ 'use strict' -var P = require('./promise') -var Pool = require('./pool') -var config = require('../config') -var localizeTimestamp = require('fxa-shared').l10n.localizeTimestamp({ +const Joi = require('joi') +const createBackendServiceAPI = require('./backendService') +const config = require('../config') +const localizeTimestamp = require('fxa-shared').l10n.localizeTimestamp({ supportedLanguages: config.get('i18n').supportedLanguages, defaultLanguage: config.get('i18n').defaultLanguage }) module.exports = function (log, error) { - const SafeUrl = require('./safe-url')(log) - const SAFE_URLS = {} + + const CustomsAPI = createBackendServiceAPI(log, config, 'customs', { + + check: { + path: '/check', + method: 'POST', + validate: { + payload: { + email: Joi.string().required(), + ip: Joi.string().required(), + action: Joi.string().required(), + headers: Joi.object().optional(), + query: Joi.object().optional(), + payload: Joi.object().optional() + }, + response: { + block: Joi.boolean().required(), + blockReason: Joi.string().optional(), + suspect: Joi.boolean().optional(), + unblock: Joi.boolean().optional(), + retryAfter: Joi.number().optional() + } + } + }, + + checkAuthenticated: { + path: '/checkAuthenticated', + method: 'POST', + validate: { + payload: { + ip: Joi.string().required(), + action: Joi.string().required(), + uid: Joi.string().required(), + }, + response: { + block: Joi.boolean().required(), + blockReason: Joi.string().optional(), + retryAfter: Joi.number().optional() + } + } + }, + + checkIpOnly: { + path: '/checkIpOnly', + method: 'POST', + validate: { + payload: { + ip: Joi.string().required(), + action: Joi.string().required(), + }, + response: { + block: Joi.boolean().required(), + blockReason: Joi.string().optional(), + suspect: Joi.boolean().optional(), + unblock: Joi.boolean().optional(), + retryAfter: Joi.number().optional() + } + } + }, + + failedLoginAttempt: { + path: '/failedLoginAttempt', + method: 'POST', + validate: { + payload: { + email: Joi.string().required(), + ip: Joi.string().required(), + errno: Joi.number().required() + }, + response: {} + } + }, + + passwordReset: { + path: '/passwordReset', + method: 'POST', + validate: { + payload: { + email: Joi.string().required(), + }, + response: {} + } + }, + + }) // Perform a deep clone of payload and remove user password. function sanitizePayload(payload) { @@ -36,49 +119,43 @@ module.exports = function (log, error) { function Customs(url) { if (url === 'none') { - this.pool = { - post: function () { return P.resolve({ block: false })}, - close: function () {} + const noblock = async function () { return { block: false }} + const noop = async function () {} + this.api = { + check: noblock, + checkAuthenticated: noblock, + checkIpOnly: noblock, + failedLoginAttempt: noop, + passwordReset: noop, + close: noop } - } - else { - this.pool = new Pool(url, { timeout: 3000 }) + } else { + this.api = new CustomsAPI(url, { timeout: 3000 }) } } - SAFE_URLS.check = new SafeUrl('/check') - Customs.prototype.check = function (request, email, action) { - log.trace({ op: 'customs.check', email: email, action: action }) - return this.pool.post( - SAFE_URLS.check, - undefined, - { - ip: request.app.clientAddress, - email: email, - action: action, - headers: request.headers, - query: request.query, - payload: sanitizePayload(request.payload) - } - ) - .then( - handleCustomsResult.bind(request), - err => { - log.error({ op: 'customs.check.1', email: email, action: action, err: err }) - throw error.backendServiceFailure('customs', 'check') - } - ) + Customs.prototype.check = async function (request, email, action) { + const result = await this.api.check({ + ip: request.app.clientAddress, + email: email, + action: action, + headers: request.headers, + query: request.query, + payload: sanitizePayload(request.payload) + }) + return handleCustomsResult(request, result) } - function handleCustomsResult (result) { - const request = this + // Annotate the request and/or throw an error + // based on the check result returned by customs-server. + function handleCustomsResult (request, result) { if (result.suspect) { request.app.isSuspiciousRequest = true } if (result.block) { - // Log a flow event that user got blocked. + // Log a flow event that the user got blocked. request.emitMetricsEvent('customs.blocked') const unblock = !! result.unblock @@ -98,97 +175,43 @@ module.exports = function (log, error) { } } - SAFE_URLS.checkAuthenticated = new SafeUrl('/checkAuthenticated') - Customs.prototype.checkAuthenticated = function (action, ip, uid) { - log.trace({ op: 'customs.checkAuthenticated', action: action, uid: uid }) - - return this.pool.post( - SAFE_URLS.checkAuthenticated, - undefined, - { - action: action, - ip: ip, - uid: uid - } - ) - .then( - function (result) { - if (result.block) { - if (result.retryAfter) { - throw error.tooManyRequests(result.retryAfter) - } - throw error.requestBlocked() - } - }, - function (err) { - log.error({ op: 'customs.checkAuthenticated', uid: uid, action: action, err: err }) - throw error.backendServiceFailure('customs', 'checkAuthenticated') - } - ) + Customs.prototype.checkAuthenticated = async function (request, uid, action) { + const result = await this.api.checkAuthenticated({ + action: action, + ip: request.app.clientAddress, + uid: uid + }) + return handleCustomsResult(request, result) } - SAFE_URLS.checkIpOnly = new SafeUrl('/checkIpOnly') - Customs.prototype.checkIpOnly = function (request, action) { - log.trace({ op: 'customs.checkIpOnly', action: action }) - return this.pool.post(SAFE_URLS.checkIpOnly, undefined, { + Customs.prototype.checkIpOnly = async function (request, action) { + const result = await this.api.checkIpOnly({ ip: request.app.clientAddress, action: action }) - .then( - handleCustomsResult.bind(request), - err => { - log.error({ op: 'customs.checkIpOnly.1', action: action, err: err }) - throw error.backendServiceFailure('customs', 'checkIpOnly') - } - ) + return handleCustomsResult(request, result) } - SAFE_URLS.failedLoginAttempt = new SafeUrl('/failedLoginAttempt') - Customs.prototype.flag = function (ip, info) { + Customs.prototype.flag = async function (ip, info) { var email = info.email var errno = info.errno || error.ERRNO.UNEXPECTED_ERROR - log.trace({ op: 'customs.flag', ip: ip, email: email, errno: errno }) - return this.pool.post( - SAFE_URLS.failedLoginAttempt, - undefined, - { - ip: ip, - email: email, - errno: errno - } - ) - .then( - // There's no useful information in the HTTP response, discard it. - function () {}, - function (err) { - log.error({ op: 'customs.flag.1', email: email, err: err }) - throw error.backendServiceFailure('customs', 'flag') - } - ) + // There's no useful information in the HTTP response, ignore it. + await this.api.failedLoginAttempt({ + ip: ip, + email: email, + errno: errno + }) } - SAFE_URLS.passwordReset = new SafeUrl('/passwordReset') - Customs.prototype.reset = function (email) { - log.trace({ op: 'customs.reset', email: email }) - return this.pool.post( - SAFE_URLS.passwordReset, - undefined, - { - email: email - } - ) - .then( - // There's no useful information in the HTTP response, discard it. - function () {}, - function (err) { - log.error({ op: 'customs.reset.1', email: email, err: err }) - throw error.backendServiceFailure('customs', 'reset') - } - ) + Customs.prototype.reset = async function (email) { + // There's no useful information in the HTTP response, ignore it. + await this.api.passwordReset({ + email: email + }) } Customs.prototype.close = function () { - return this.pool.close() + return this.api.close() } return Customs diff --git a/lib/error.js b/lib/error.js index c7de19f9..2c647823 100644 --- a/lib/error.js +++ b/lib/error.js @@ -78,6 +78,8 @@ var ERRNO = { SERVER_BUSY: 201, FEATURE_NOT_ENABLED: 202, BACKEND_SERVICE_FAILURE: 203, + + INTERNAL_VALIDATION_ERROR: 998, UNEXPECTED_ERROR: 999 } @@ -861,6 +863,15 @@ AppError.backendServiceFailure = (service, operation) => { }) } +AppError.internalValidationError = () => { + return new AppError({ + code: 500, + error: 'Internal Server Error', + errno: ERRNO.INTERNAL_VALIDATION_ERROR, + message: 'An internal validation check failed.' + }) +} + AppError.unexpectedError = () => { return new AppError({}) } diff --git a/lib/pushbox.js b/lib/pushbox.js index 5b7cfe84..2db14aef 100644 --- a/lib/pushbox.js +++ b/lib/pushbox.js @@ -18,15 +18,11 @@ const isA = require('joi') const error = require('./error') -const Pool = require('./pool') -const P = require('./promise') +const createBackendServiceAPI = require('./backendService') const validators = require('./routes/validators') const base64url = require('base64url') -const LOG_OP_RETRIEVE = 'pushbox.retrieve' -const LOG_OP_STORE = 'pushbox.store' - const PUSHBOX_RETRIEVE_SCHEMA = isA.object({ last: isA.boolean().optional(), index: isA.number().optional(), @@ -44,15 +40,6 @@ const PUSHBOX_STORE_SCHEMA = isA.object({ status: isA.number().required() }).or('index', 'error') -const validateRetrieveResponse = P.promisify(PUSHBOX_RETRIEVE_SCHEMA.validate, { - context: PUSHBOX_RETRIEVE_SCHEMA -}) - -const validateStoreResponse = P.promisify(PUSHBOX_STORE_SCHEMA.validate, { - context: PUSHBOX_STORE_SCHEMA -}) - - // Pushbox stores strings, so these are a little pair // of helper functions to allow us to store arbitrary // JSON-serializable objects. @@ -78,14 +65,50 @@ module.exports = function (log, config) { } } - const pool = new Pool(config.pushbox.url, { timeout: 15000 }) + const PushboxAPI = createBackendServiceAPI(log, config, 'pushbox', { + + retrieve: { + path: '/v1/store/:uid/:deviceId', + method: 'GET', + validate: { + params: { + uid: isA.string().regex(validators.HEX_STRING).required(), + deviceId: isA.string().regex(validators.HEX_STRING).required() + }, + query: { + limit: isA.string().regex(validators.DIGITS).required(), + index: isA.string().regex(validators.DIGITS).optional() + }, + response: PUSHBOX_RETRIEVE_SCHEMA + } + }, + + store: { + path: '/v1/store/:uid/:deviceId', + method: 'POST', + validate: { + params: { + uid: isA.string().regex(validators.HEX_STRING).required(), + deviceId: isA.string().regex(validators.HEX_STRING).required() + }, + payload: { + data: isA.string().required(), + ttl: isA.number().required() + }, + response: PUSHBOX_STORE_SCHEMA + } + }, + + }) + + const api = new PushboxAPI(config.pushbox.url, { + headers: {Authorization: `FxA-Server-Key ${config.pushbox.key}`}, + timeout: 15000 + }) + // pushbox expects this in seconds, not millis. const maxTTL = Math.round(config.pushbox.maxTTL / 1000) - const SafeUrl = require('./safe-url')(log) - const path = new SafeUrl('/v1/store/:uid/:deviceId') - const headers = {Authorization: `FxA-Server-Key ${config.pushbox.key}`} - return { /** * Retrieves enqueued items for a specific device. @@ -99,45 +122,29 @@ module.exports = function (log, config) { * @param {String} [index] * @returns {Promise} */ - retrieve (uid, deviceId, limit, index) { - log.trace({ - op: LOG_OP_RETRIEVE, - uid, - deviceId, - index, - limit - }) + async retrieve (uid, deviceId, limit, index) { const query = { limit: limit.toString() } if (index) { query.index = index.toString() } - const params = {uid, deviceId} - return pool.get(path, params, {query, headers}) - .then(body => { - log.info({ op: 'pushbox.retrieve.response', body: body }) - return validateRetrieveResponse(body).catch(e => { - log.error({ op: 'pushbox.retrieve', error: 'response schema validation failed', body: body }) - throw error.unexpectedError() + const body = await api.retrieve(uid, deviceId, query) + log.info({ op: 'pushbox.retrieve.response', body: body }) + if (body.error) { + log.error({ op: 'pushbox.retrieve', status: body.status, error: body.error }) + throw error.backendServiceFailure() + } + return { + last: body.last, + index: body.index, + messages: (! body.messages) ? undefined : body.messages.map(msg => { + return { + index: msg.index, + data: decodeFromStorage(msg.data) + } }) - }) - .then(body => { - if (body.error) { - log.error({ op: 'pushbox.retrieve', status: body.status, error: body.error }) - throw error.unexpectedError() - } - return { - last: body.last, - index: body.index, - messages: (! body.messages) ? undefined : body.messages.map(msg => { - return { - index: msg.index, - data: decodeFromStorage(msg.data) - } - }) - } - }) + } }, /** @@ -151,32 +158,17 @@ module.exports = function (log, config) { * @param {Object} data - data object to serialize into storage * @returns {Promise} direct url to the stored message */ - store (uid, deviceId, data, ttl) { + async store (uid, deviceId, data, ttl) { if (typeof ttl === 'undefined' || ttl > maxTTL) { ttl = maxTTL } - log.trace({ - op: LOG_OP_STORE, - uid, - deviceId, - }) - const body = {data: encodeForStorage(data), ttl} - const params = {uid, deviceId} - return pool.post(path, params, body, {headers}) - .then(body => { - log.info({ op: 'pushbox.store.response', body: body }) - return validateStoreResponse(body).catch(e => { - log.error({ op: 'pushbox.store', error: 'response schema validation failed', body: body }) - throw error.unexpectedError() - }) - }) - .then(body => { - if (body.error) { - log.error({ op: 'pushbox.store', status: body.status, error: body.error }) - throw error.unexpectedError() - } - return body - }) + const body = await api.store(uid, deviceId, {data: encodeForStorage(data), ttl}) + log.info({ op: 'pushbox.store.response', body: body }) + if (body.error) { + log.error({ op: 'pushbox.store', status: body.status, error: body.error }) + throw error.backendServiceFailure() + } + return body } } } diff --git a/lib/routes/devices-and-sessions.js b/lib/routes/devices-and-sessions.js index 4ec72c17..5a5d191c 100644 --- a/lib/routes/devices-and-sessions.js +++ b/lib/routes/devices-and-sessions.js @@ -307,9 +307,8 @@ module.exports = (log, db, config, customs, push, pushbox, devices) => { const sessionToken = request.auth.credentials const uid = sessionToken.uid const sender = sessionToken.deviceId - const ip = request.app.clientAddress - return customs.checkAuthenticated('invokeDeviceCommand', ip, uid) + return customs.checkAuthenticated(request, uid, 'invokeDeviceCommand') .then(() => db.device(uid, target)) .then(device => { if (! device.availableCommands.hasOwnProperty(command)) { @@ -375,7 +374,6 @@ module.exports = (log, db, config, customs, push, pushbox, devices) => { const body = request.payload const sessionToken = request.auth.credentials const uid = sessionToken.uid - const ip = request.app.clientAddress const payload = body.payload const endpointAction = body._endpointAction || 'devicesNotify' @@ -392,7 +390,7 @@ module.exports = (log, db, config, customs, push, pushbox, devices) => { pushOptions.TTL = body.TTL } - return customs.checkAuthenticated(endpointAction, ip, uid) + return customs.checkAuthenticated(request, uid, endpointAction) .then(() => request.app.devices) .then(devices => { if (body.to !== 'all') { diff --git a/lib/routes/recovery-key.js b/lib/routes/recovery-key.js index 672cde8c..3e0f7628 100644 --- a/lib/routes/recovery-key.js +++ b/lib/routes/recovery-key.js @@ -95,11 +95,10 @@ module.exports = (log, db, Password, verifierVersion, customs, mailer) => { log.begin('getRecoveryKey', request) const uid = request.auth.credentials.uid - const ip = request.app.clientAddress const recoveryKeyId = request.params.recoveryKeyId let recoveryData - return customs.checkAuthenticated('getRecoveryKey', ip, uid) + return customs.checkAuthenticated(request, uid, 'getRecoveryKey') .then(getRecoveryKey) .then(() => { return {recoveryData} diff --git a/lib/safe-url.js b/lib/safe-url.js index 730fee94..2b3d2506 100644 --- a/lib/safe-url.js +++ b/lib/safe-url.js @@ -46,6 +46,10 @@ module.exports = log => class SafeUrl { this._caller = caller } + params () { + return this._expectedKeys.array.slice(0) + } + render (params = {}, query = {}) { const paramsKeys = Object.keys(params) const { array: expected, set: expectedSet } = this._expectedKeys @@ -83,6 +87,6 @@ module.exports = log => class SafeUrl { _fail (op, data) { log.error(Object.assign({ op, caller: this._caller }, data)) - throw error.unexpectedError() + throw error.internalValidationError() } } diff --git a/test/local/backendService.js b/test/local/backendService.js new file mode 100644 index 00000000..5452653d --- /dev/null +++ b/test/local/backendService.js @@ -0,0 +1,314 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +'use strict' + +const nock = require('nock') +const Joi = require('joi') +const { assert } = require('chai') +const { mockLog } = require('../mocks') + +const error = require('../../lib/error') +const createBackendServiceAPI = require('../../lib/backendService') + +const mockConfig = {} + +const mockServiceURL = 'http://mock.service' +const mockService = nock(mockServiceURL) + +describe('createBackendServiceAPI', () => { + + let Service, api, log + + beforeEach(() => { + log = mockLog() + Service = createBackendServiceAPI(log, mockConfig, 'mock-service', { + + testSimpleGet: { + method: 'GET', + path: '/test_get/:first/:second' + }, + + testSimplePost: { + method: 'POST', + path: '/test_post/:id', + validate: { + payload: { + foo: Joi.string().required() + } + } + }, + + testGetWithValidation: { + method: 'GET', + path: '/test_get/:first/:second', + validate: { + params: { + first: Joi.string().regex(/[a-z]+/).required(), + second: Joi.string().required() + }, + query: { + foo: Joi.string().optional() + }, + response: { + status: Joi.number().required(), + message: Joi.string().required() + } + } + }, + + testPostWithValidation: { + method: 'POST', + path: '/test_post/:id', + validate: { + params: { + id: Joi.string().regex(/[a-z]+/).required() + }, + query: { + bar: Joi.string().optional() + }, + payload: { + foo: Joi.string().required() + }, + response: { + status: Joi.number().required(), + message: Joi.string().required() + } + } + } + + }) + api = new Service(mockServiceURL) + }) + + afterEach(() => { + assert.ok(nock.isDone(), 'there should be no pending request mocks at the end of a test') + }) + + it('can make a simple GET request and return the response', async () => { + mockService.get('/test_get/one/two') + .reply(200, { + hello: 'world' + }) + const resp = await api.testSimpleGet('one', 'two') + assert.deepEqual(resp, { + hello: 'world' + }) + }) + + it('can make a simple POST request and return the response', async () => { + mockService.post('/test_post/abc') + .reply(200, { + hello: 'world' + }) + const resp = await api.testSimplePost('abc', { foo: 'bar' }) + assert.deepEqual(resp, { + hello: 'world' + }) + }) + + it('requires that a body be provided for POST requests', async () => { + try { + await api.testSimplePost('abc') + assert.fail('should have thrown') + } catch (err) { + assert.equal(err.message, 'mock-service.testSimplePost must be called with 2 arguments (1 given)') + } + }) + + it('validates the request body', async () => { + try { + await api.testSimplePost('abc', { foo: 123 }) + assert.fail('should have thrown') + } catch (err) { + assert.equal(err.errno, error.ERRNO.INTERNAL_VALIDATION_ERROR) + assert.equal(log.error.callCount, 1, 'an error was logged') + assert.equal(log.error.getCall(0).args[0].op, 'mock-service.testSimplePost') + assert.equal(log.error.getCall(0).args[0].error, 'request schema validation failed') + assert.ok(/"foo" must be a string/.test(log.error.getCall(0).args[0].message)) + } + }) + + it('validates path parameters', async () => { + try { + await api.testGetWithValidation('ABC', '123', {}) + assert.fail('should have thrown') + } catch (err) { + assert.equal(err.errno, error.ERRNO.INTERNAL_VALIDATION_ERROR) + assert.equal(log.error.callCount, 1, 'an error was logged') + assert.equal(log.error.getCall(0).args[0].op, 'mock-service.testGetWithValidation') + assert.equal(log.error.getCall(0).args[0].error, 'params schema validation failed') + assert.ok(/fails to match the required pattern/.test(log.error.getCall(0).args[0].message)) + } + log.error.reset() + try { + await api.testGetWithValidation('abc', 123, {}) + assert.fail('should have thrown') + } catch (err) { + assert.equal(err.errno, error.ERRNO.INTERNAL_VALIDATION_ERROR) + assert.equal(log.error.callCount, 1, 'an error was logged') + assert.equal(log.error.getCall(0).args[0].op, 'mock-service.testGetWithValidation') + assert.equal(log.error.getCall(0).args[0].error, 'params schema validation failed') + assert.ok(/"second" must be a string/.test(log.error.getCall(0).args[0].message)) + } + }) + + it('rejects unsafe path parameters', async () => { + try { + await api.testSimpleGet('abc\n', '123') + assert.fail('should have thrown') + } catch (err) { + assert.equal(err.errno, error.ERRNO.INTERNAL_VALIDATION_ERROR) + assert.equal(log.error.callCount, 1, 'an error was logged') + assert.equal(log.error.getCall(0).args[0].op, 'safeUrl.unsafe') + assert.equal(log.error.getCall(0).args[0].key, 'first') + } + }) + + it('validates query paramters', async () => { + try { + await api.testGetWithValidation('abc', '123', { foo: 123 }) + assert.fail('should have thrown') + } catch (err) { + assert.equal(err.errno, error.ERRNO.INTERNAL_VALIDATION_ERROR) + assert.equal(log.error.callCount, 1, 'an error was logged') + assert.equal(log.error.getCall(0).args[0].op, 'mock-service.testGetWithValidation') + assert.equal(log.error.getCall(0).args[0].error, 'query schema validation failed') + assert.ok(/"foo" must be a string/.test(log.error.getCall(0).args[0].message)) + } + }) + + it('rejects unsafe query parameters', async () => { + try { + await api.testGetWithValidation('abc', '123', { foo: '123\n' }) + assert.fail('should have thrown') + } catch (err) { + assert.equal(err.errno, error.ERRNO.INTERNAL_VALIDATION_ERROR) + assert.equal(log.error.callCount, 1, 'an error was logged') + assert.equal(log.error.getCall(0).args[0].op, 'safeUrl.unsafe') + assert.equal(log.error.getCall(0).args[0].key, 'foo') + } + }) + + it('requires that query parameters be present if schema is declared', async () => { + try { + await api.testGetWithValidation('abc', '123') + assert.fail('should have thrown') + } catch (err) { + assert.equal(err.message, 'mock-service.testGetWithValidation must be called with 3 arguments (2 given)') + } + }) + + it('validates response body', async () => { + let requestBody + mockService.post('/test_post/abc', body => { + requestBody = body + return true + }) + .query({ bar: 'baz' }) + .reply(200, { + status: 200, + message: 'ok' + }) + const resp = await api.testPostWithValidation('abc', { bar: 'baz' }, { foo: 'bar' }) + assert.deepEqual(requestBody, { + foo: 'bar' + }) + assert.deepEqual(resp, { + status: 200, + message: 'ok' + }) + + mockService.post('/test_post/abc', () => true) + .query({ bar: 'baz' }) + .reply(200, { + status: 'whoops', + message: 'whoops' + }) + try { + await api.testPostWithValidation('abc', { bar: 'baz' }, { foo: 'bar' }) + assert.fail('should have thrown') + } catch (err) { + assert.equal(err.errno, error.ERRNO.INTERNAL_VALIDATION_ERROR) + assert.equal(log.error.callCount, 1, 'an error was logged') + assert.equal(log.error.getCall(0).args[0].op, 'mock-service.testPostWithValidation') + assert.equal(log.error.getCall(0).args[0].error, 'response schema validation failed') + assert.ok(/"status" must be a number/.test(log.error.getCall(0).args[0].message)) + } + }) + + it('strips unknown keys from response body', async () => { + let requestBody + mockService.post('/test_post/abc', body => { + requestBody = body + return true + }) + .query({ bar: 'baz' }) + .reply(200, { + status: 200, + message: 'ok', + something: 'extra' + }) + const resp = await api.testPostWithValidation('abc', { bar: 'baz' }, { foo: 'bar' }) + assert.deepEqual(requestBody, { + foo: 'bar' + }) + assert.deepEqual(resp, { + status: 200, + message: 'ok' + }) + }) + + it('re-throws 400-level errors returned by the service', async () => { + mockService.post('/test_post/abc', body => true) + .reply(400, { + message: 'invalid frobble', + }) + try { + await api.testPostWithValidation('abc', {}, { foo: 'bar'}) + assert.fail('should have thrown') + } catch (err) { + assert.equal(err.statusCode, 400) + assert.equal(err.message, 'invalid frobble') + } + }) + + it('logs 500-level errors and returns backendServiceFailure', async () => { + mockService.post('/test_post/abc', body => true) + .reply(500, { + message: 'invalid frobble', + }) + try { + await api.testPostWithValidation('abc', {}, { foo: 'bar'}) + assert.fail('should have thrown') + } catch (err) { + assert.equal(err.errno, error.ERRNO.BACKEND_SERVICE_FAILURE) + assert.equal(log.error.callCount, 1, 'an error was logged') + assert.equal(log.error.getCall(0).args[0].op, 'mock-service.testPostWithValidation.1') + assert.deepEqual(log.error.getCall(0).args[0].params, { id: 'abc' }) + assert.deepEqual(log.error.getCall(0).args[0].query, {}) + assert.deepEqual(log.error.getCall(0).args[0].payload, { foo: 'bar' }) + assert.deepEqual(log.error.getCall(0).args[0].err.message, 'invalid frobble') + } + }) + + it('logs connection errors and returns backendServiceFailure', async () => { + mockService.post('/test_post/abc', body => true) + .replyWithError('ruh-roh!') + try { + await api.testPostWithValidation('abc', {}, { foo: 'bar'}) + assert.fail('should have thrown') + } catch (err) { + assert.equal(err.errno, error.ERRNO.BACKEND_SERVICE_FAILURE) + assert.equal(log.error.callCount, 1, 'an error was logged') + assert.equal(log.error.getCall(0).args[0].op, 'mock-service.testPostWithValidation.1') + assert.deepEqual(log.error.getCall(0).args[0].params, { id: 'abc' }) + assert.deepEqual(log.error.getCall(0).args[0].query, {}) + assert.deepEqual(log.error.getCall(0).args[0].payload, { foo: 'bar' }) + assert.ok(log.error.getCall(0).args[0].err.message.indexOf('ruh-roh!') >= 0) + } + }) + + +}) diff --git a/test/local/customs.js b/test/local/customs.js index fe9815b7..3497e3e0 100644 --- a/test/local/customs.js +++ b/test/local/customs.js @@ -350,26 +350,26 @@ describe('Customs', () => { .post('/checkAuthenticated', checkRequestBody).reply(200, '{"block":false,"retryAfter":0}') .post('/checkAuthenticated', checkRequestBody).reply(200, '{"block":true,"retryAfter":10001}') - return customsWithUrl.checkAuthenticated(action, ip, uid) + return customsWithUrl.checkAuthenticated(request, uid, action) .then(function(result) { assert.equal(result, undefined, 'Nothing is returned when /checkAuthenticated succeeds - 1') - return customsWithUrl.checkAuthenticated(action, ip, uid) + return customsWithUrl.checkAuthenticated(request, uid, action) }) .then(function(result) { assert.equal(result, undefined, 'Nothing is returned when /checkAuthenticated succeeds - 2') - return customsWithUrl.checkAuthenticated(action, ip, uid) + return customsWithUrl.checkAuthenticated(request, uid, action) }) .then(function(result) { assert.equal(result, undefined, 'Nothing is returned when /checkAuthenticated succeeds - 3') - return customsWithUrl.checkAuthenticated(action, ip, uid) + return customsWithUrl.checkAuthenticated(request, uid, action) }) .then(function(result) { assert.equal(result, undefined, 'Nothing is returned when /checkAuthenticated succeeds - 4') - return customsWithUrl.checkAuthenticated(action, ip, uid) + return customsWithUrl.checkAuthenticated(request, uid, action) }) .then(function() { // request is blocked - return customsWithUrl.checkAuthenticated(action, ip, uid) + return customsWithUrl.checkAuthenticated(request, uid, action) }) .then(function() { assert(false, 'This should have failed the check since it should be blocked') diff --git a/test/local/pushbox.js b/test/local/pushbox.js index 2904c3e8..6dc44e9d 100644 --- a/test/local/pushbox.js +++ b/test/local/pushbox.js @@ -4,12 +4,12 @@ 'use strict' -const ROOT_DIR = '../..' - const { assert } = require('chai') -const proxyquire = require('proxyquire') -const sinon = require('sinon') +const nock = require('nock') +const pushboxModule = require('../../lib/pushbox') +const error = require('../../lib/error') const {mockLog} = require('../mocks') + const mockConfig = { publicUrl: 'https://accounts.example.com', pushbox: { @@ -19,41 +19,40 @@ const mockConfig = { maxTTL: 123456000 } } -const mockDeviceIds = ['bogusid1', 'bogusid2', 'bogusid3'] +const mockDeviceIds = ['AAAA11', 'BBBB22', 'CCCC33'] const mockData = 'eyJmb28iOiAiYmFyIn0' -const mockUid = 'myuid' -const pushboxModulePath = `${ROOT_DIR}/lib/pushbox` +const mockUid = 'ABCDEF' + +const mockPushboxServer = nock(mockConfig.pushbox.url, { + reqheaders: {Authorization: `FxA-Server-Key ${mockConfig.pushbox.key}`} +}).defaultReplyHeaders({ + 'Content-Type': 'application/json' +}) describe('pushbox', () => { + + afterEach(() => { + assert.ok(nock.isDone(), 'there should be no pending request mocks at the end of a test') + }) + it( 'retrieve', () => { - const FakePool = function() {} - const getSpy = sinon.spy(() => Promise.resolve({ - status: 200, - last: true, - index: '15', - messages: [{ - index: '15', - // This is { foo: "bar", bar: "bar" }, encoded. - data: 'eyJmb28iOiJiYXIiLCAiYmFyIjogImJhciJ9' - }] - })) - FakePool.prototype.get = getSpy - const mocks = { - './pool': FakePool - } - const pushbox = proxyquire(pushboxModulePath, mocks)(mockLog(), mockConfig) - + mockPushboxServer.get(`/v1/store/${mockUid}/${mockDeviceIds[0]}`) + .query({ limit: 50, index: 10 }) + .reply(200, { + status: 200, + last: true, + index: '15', + messages: [{ + index: '15', + // This is { foo: "bar", bar: "bar" }, encoded. + data: 'eyJmb28iOiJiYXIiLCAiYmFyIjogImJhciJ9' + }] + }) + const pushbox = pushboxModule(mockLog(), mockConfig) return pushbox.retrieve(mockUid, mockDeviceIds[0], 50, 10) .then(resp => { - assert.equal(getSpy.callCount, 1, 'get request was made') - const args = getSpy.args[0] - assert.equal(args.length, 3) - assert.equal(args[0]._template.toString(), '/v1/store/:uid/:deviceId') - assert.deepEqual(args[1], {uid: mockUid, deviceId: mockDeviceIds[0]}) - assert.deepEqual(args[2], {query: {limit:'50', index:'10'}, headers: {Authorization: `FxA-Server-Key ${mockConfig.pushbox.key}`}}) - assert.deepEqual(resp, { last: true, index: 15, @@ -69,21 +68,17 @@ describe('pushbox', () => { it( 'retrieve validates the pushbox server response', () => { - const FakePool = function() {} - const getSpy = sinon.spy(() => Promise.resolve({ + mockPushboxServer.get(`/v1/store/${mockUid}/${mockDeviceIds[0]}`) + .query({ limit: 50, index: 10 }) + .reply(200, { 'bogus':'object' - })) - FakePool.prototype.get = getSpy - const mocks = { - './pool': FakePool - } + }) const log = mockLog() - const pushbox = proxyquire(pushboxModulePath, mocks)(log, mockConfig) - + const pushbox = pushboxModule(log, mockConfig) return pushbox.retrieve(mockUid, mockDeviceIds[0], 50, 10) .then(() => assert.ok(false, 'should not happen'), (err) => { assert.ok(err) - assert.equal(err.errno, 999) + assert.equal(err.errno, error.ERRNO.INTERNAL_VALIDATION_ERROR) assert.equal(log.error.callCount, 1, 'an error was logged') assert.equal(log.error.getCall(0).args[0].op, 'pushbox.retrieve') assert.equal(log.error.getCall(0).args[0].error, 'response schema validation failed') @@ -94,22 +89,18 @@ describe('pushbox', () => { it( 'retrieve throws on error response', () => { - const FakePool = function() {} - const getSpy = sinon.spy(() => Promise.resolve({ + mockPushboxServer.get(`/v1/store/${mockUid}/${mockDeviceIds[0]}`) + .query({ limit: 50, index: 10 }) + .reply(200, { 'error': 'lamentably, an error hath occurred', status: 1234 - })) - FakePool.prototype.get = getSpy - const mocks = { - './pool': FakePool - } + }) const log = mockLog() - const pushbox = proxyquire(pushboxModulePath, mocks)(log, mockConfig) - + const pushbox = pushboxModule(log, mockConfig) return pushbox.retrieve(mockUid, mockDeviceIds[0], 50, 10) .then(() => assert.ok(false, 'should not happen'), (err) => { assert.ok(err) - assert.equal(err.errno, 999) + assert.equal(err.errno, error.ERRNO.BACKEND_SERVICE_FAILURE) assert.equal(log.error.callCount, 1, 'an error was logged') assert.equal(log.error.getCall(0).args[0].op, 'pushbox.retrieve') assert.equal(log.error.getCall(0).args[0].error, 'lamentably, an error hath occurred') @@ -121,27 +112,19 @@ describe('pushbox', () => { it( 'store', () => { - const FakePool = function() {} - const postSpy = sinon.spy(() => Promise.resolve({ + let requestBody + mockPushboxServer.post(`/v1/store/${mockUid}/${mockDeviceIds[0]}`, body => { + requestBody = body + return true + }) + .reply(200, { status: 200, index: '12' - })) - FakePool.prototype.post = postSpy - const mocks = { - './pool': FakePool - } - const pushbox = proxyquire(pushboxModulePath, mocks)(mockLog(), mockConfig) - + }) + const pushbox = pushboxModule(mockLog(), mockConfig) return pushbox.store(mockUid, mockDeviceIds[0], { test: 'data' }) .then(({index}) => { - assert.equal(postSpy.callCount, 1, 'post request was made') - const args = postSpy.args[0] - assert.equal(args.length, 4) - assert.equal(args[0]._template.toString(), '/v1/store/:uid/:deviceId') - assert.deepEqual(args[1], {uid: mockUid, deviceId: mockDeviceIds[0]}) - assert.deepEqual(args[2], {data: 'eyJ0ZXN0IjoiZGF0YSJ9', ttl: 123456}) - assert.deepEqual(args[3], {headers: {Authorization: `FxA-Server-Key ${mockConfig.pushbox.key}`}}) - + assert.deepEqual(requestBody, {data: 'eyJ0ZXN0IjoiZGF0YSJ9', ttl: 123456}) assert.equal(index, '12') }) } @@ -150,23 +133,19 @@ describe('pushbox', () => { it( 'store with custom ttl', () => { - const FakePool = function() {} - const postSpy = sinon.spy(() => Promise.resolve({ + let requestBody + mockPushboxServer.post(`/v1/store/${mockUid}/${mockDeviceIds[0]}`, body => { + requestBody = body + return true + }) + .reply(200, { status: 200, index: '12' - })) - FakePool.prototype.post = postSpy - const mocks = { - './pool': FakePool - } - const pushbox = proxyquire(pushboxModulePath, mocks)(mockLog(), mockConfig) - + }) + const pushbox = pushboxModule(mockLog(), mockConfig) return pushbox.store(mockUid, mockDeviceIds[0], { test: 'data' }, 42) .then(({index}) => { - assert.equal(postSpy.callCount, 1, 'post request was made') - const args = postSpy.args[0] - assert.deepEqual(args[2], {data: 'eyJ0ZXN0IjoiZGF0YSJ9', ttl: 42}) - + assert.deepEqual(requestBody, {data: 'eyJ0ZXN0IjoiZGF0YSJ9', ttl: 42}) assert.equal(index, '12') }) } @@ -175,23 +154,19 @@ describe('pushbox', () => { it( 'store caps ttl at configured maximum', () => { - const FakePool = function() {} - const postSpy = sinon.spy(() => Promise.resolve({ + let requestBody + mockPushboxServer.post(`/v1/store/${mockUid}/${mockDeviceIds[0]}`, body => { + requestBody = body + return true + }) + .reply(200, { status: 200, index: '12' - })) - FakePool.prototype.post = postSpy - const mocks = { - './pool': FakePool - } - const pushbox = proxyquire(pushboxModulePath, mocks)(mockLog(), mockConfig) - + }) + const pushbox = pushboxModule(mockLog(), mockConfig) return pushbox.store(mockUid, mockDeviceIds[0], { test: 'data' }, 999999999) .then(({index}) => { - assert.equal(postSpy.callCount, 1, 'post request was made') - const args = postSpy.args[0] - assert.deepEqual(args[2], {data: 'eyJ0ZXN0IjoiZGF0YSJ9', ttl: 123456}) - + assert.deepEqual(requestBody, {data: 'eyJ0ZXN0IjoiZGF0YSJ9', ttl: 123456}) assert.equal(index, '12') }) } @@ -200,21 +175,16 @@ describe('pushbox', () => { it( 'store validates the pushbox server response', () => { - const FakePool = function() {} - const postSpy = sinon.spy(() => Promise.resolve({ + mockPushboxServer.post(`/v1/store/${mockUid}/${mockDeviceIds[0]}`) + .reply(200, { 'bogus':'object' - })) - FakePool.prototype.post = postSpy - const mocks = { - './pool': FakePool - } + }) const log = mockLog() - const pushbox = proxyquire(pushboxModulePath, mocks)(log, mockConfig) - + const pushbox = pushboxModule(log, mockConfig) return pushbox.store(mockUid, mockDeviceIds[0], { test: 'data' }) .then(() => assert.ok(false, 'should not happen'), (err) => { assert.ok(err) - assert.equal(err.errno, 999) + assert.equal(err.errno, error.ERRNO.INTERNAL_VALIDATION_ERROR) assert.equal(log.error.callCount, 1, 'an error was logged') assert.equal(log.error.getCall(0).args[0].op, 'pushbox.store') assert.equal(log.error.getCall(0).args[0].error, 'response schema validation failed') @@ -225,22 +195,17 @@ describe('pushbox', () => { it( 'retrieve throws on error response', () => { - const FakePool = function() {} - const postSpy = sinon.spy(() => Promise.resolve({ + mockPushboxServer.post(`/v1/store/${mockUid}/${mockDeviceIds[0]}`) + .reply(200, { 'error': 'Alas, an error! I knew it, Horatio.', 'status': 789 - })) - FakePool.prototype.post = postSpy - const mocks = { - './pool': FakePool - } + }) const log = mockLog() - const pushbox = proxyquire(pushboxModulePath, mocks)(log, mockConfig) - + const pushbox = pushboxModule(log, mockConfig) return pushbox.store(mockUid, mockDeviceIds[0], { test: 'data' }) .then(() => assert.ok(false, 'should not happen'), (err) => { assert.ok(err) - assert.equal(err.errno, 999) + assert.equal(err.errno, error.ERRNO.BACKEND_SERVICE_FAILURE) assert.equal(log.error.callCount, 1, 'an error was logged') assert.equal(log.error.getCall(0).args[0].op, 'pushbox.store') assert.equal(log.error.getCall(0).args[0].error, 'Alas, an error! I knew it, Horatio.') @@ -252,19 +217,10 @@ describe('pushbox', () => { it( 'feature disabled', () => { - const FakePool = function() {} - const postSpy = sinon.spy() - const getSpy = sinon.spy() - FakePool.prototype.post = postSpy - FakePool.prototype.get = getSpy - const mocks = { - './pool': FakePool - } const config = Object.assign({}, mockConfig, { pushbox: {enabled: false} }) - const pushbox = proxyquire(pushboxModulePath, mocks)(mockLog(), config) - + const pushbox = pushboxModule(mockLog(), config) return pushbox.store(mockUid, mockDeviceIds[0], 'sendtab', mockData) .then(() => assert.ok(false, 'should not happen'), (err) => { assert.ok(err) diff --git a/test/local/routes/recovery-keys.js b/test/local/routes/recovery-keys.js index ca1b1870..e6c18a0e 100644 --- a/test/local/routes/recovery-keys.js +++ b/test/local/routes/recovery-keys.js @@ -112,9 +112,9 @@ describe('GET /recoveryKey/{recoveryKeyId}', () => { assert.equal(customs.checkAuthenticated.callCount, 1) const args = customs.checkAuthenticated.args[0] assert.equal(args.length, 3) - assert.equal(args[0], 'getRecoveryKey') - assert.equal(args[1], request.app.clientAddress) - assert.equal(args[2], uid) + assert.deepEqual(args[0], request) + assert.equal(args[1], uid) + assert.equal(args[2], 'getRecoveryKey') }) it('called db.getRecoveryKey correctly', () => {