From 8da6ebecff2a38899221553c4795f5f974f7b59e Mon Sep 17 00:00:00 2001 From: Aubrey Keus Date: Fri, 22 Nov 2019 08:54:43 -0500 Subject: [PATCH 1/3] Add Android feedback collection endpoint * Add an endpoint to collect feedback from a mobile device * Store feedback in Mongo * Add script to remove feedback entries by id or date * Add tests --- bin/remove-feedback.js | 51 ++++++++++++++++++++++ src/controllers/feedback.js | 84 +++++++++++++++++++++++++++++++++++++ src/index.js | 5 ++- test/feedback.js | 26 ++++++++++++ 4 files changed, 165 insertions(+), 1 deletion(-) create mode 100755 bin/remove-feedback.js create mode 100644 src/controllers/feedback.js create mode 100644 test/feedback.js diff --git a/bin/remove-feedback.js b/bin/remove-feedback.js new file mode 100755 index 00000000..45fc5e53 --- /dev/null +++ b/bin/remove-feedback.js @@ -0,0 +1,51 @@ +#!/usr/bin/env node + +const assert = require('assert') +const moment = require('moment') + +const MongoClient = require('mongodb').MongoClient +const mongoURL = process.env.MLAB_URI +if (!mongoURL) throw new Error('MLAB_URI must be set in environment') + +const FEEDBACK_COLLECTION = process.env.FEEDBACK_COLLECTION || 'feedback' + +const args = require('yargs') + .describe('id', 'remove a single item by id') + .describe('interval', 'remove all feedback created before this interval') + .argv + +const main = async (args) => { + MongoClient.connect(mongoURL, (err, connection) => { + const collection = connection.collection(FEEDBACK_COLLECTION); + if (args.id) { + collection.deleteOne({ id : args.id }, (err, result) => { + assert.equal(err, null) + if (result.result.n === 0) { + console.log(`Feedback with id ${args.id} not found`) + } else { + console.log(`Feedback with id ${args.id} removed`) + } + connection.close() + }) + } else if (args.interval) { + let [num, period] = args.interval.split(' ') + num = parseInt(num) + const targetYMD = moment().subtract(num, period).format('YYYY-MM-DD') + console.log(`This will remove all feedback before ${targetYMD}. Run again with -f to accept`) + if (targetYMD && args.f) { + console.log(`removing all feedback before ${targetYMD}`) + collection.deleteMany({ ymd: { $lt: targetYMD }}, (err, result) => { + assert.equal(err, null) + console.log(`${result.result.n} feedback records removed`) + connection.close() + }) + } else { + connection.close() + } + } else { + connection.close() + } + }) +} + +main(args) diff --git a/src/controllers/feedback.js b/src/controllers/feedback.js new file mode 100644 index 00000000..4b40789a --- /dev/null +++ b/src/controllers/feedback.js @@ -0,0 +1,84 @@ +const Joi = require('joi') +const Boom = require('boom') +const moment = require('moment') +const storage = require('../storage') +const uuid = require('uuid/v4') + +const FEEDBACK_COLLECTION = process.env.FEEDBACK_COLLECTION || 'feedback' + +// feedback validator +const validator = { + payload: { + selection: Joi.any().allow('yes', 'no').required(), + platform: Joi.string().required(), + os_version: Joi.string().required(), + phone_make: Joi.string().required(), + phone_model: Joi.string().required(), + phone_arch: Joi.string().required(), + app_version: Joi.string().required(), + user_feedback: Joi.string().max(1024).optional(), + api_key: Joi.string().required(), + } +} + +// build object to be stored from feedback +const buildStorageObject = (payload) => { + return { + id: uuid(), + ts: (new Date()).getTime(), + ymd: moment().format('YYYY-MM-DD'), + selection: payload.selection, + platform: payload.platform, + os_version: payload.os_version, + phone_make: payload.phone_make, + phone_model: payload.phone_model, + phone_arch: payload.phone_arch, + version: payload.app_version, + user_feedback: payload.user_feedback, + } +} + +// build return result object +const successResult = (id) => { + return { + id: id, + status: 'ok', + ts: (new Date()).getTime() + } +} + +exports.setup = (runtime) => { + const routes = [] + + routes.push({ + method: 'POST', + path: '/1/feedback', + config: { + description: '* Record feedback', + handler: async (request, reply) => { + try { + // phase 2 - to be implemented - rate limit on IP address + + // phase 2 - to be implemented - callout to referral server to verify api key + + // build event + const storageObject = buildStorageObject(request.payload) + + // abstract storage mechanism + await storage.storeObjectOrEvent(runtime, FEEDBACK_COLLECTION, storageObject) + + // return success + return reply(successResult(storageObject.id)) + } catch (e) { + return reply(Boom.badImplementation(e.toString())) + } + }, + validate: validator + } + }) + + return routes +} + +exports.buildStorageObject = buildStorageObject +exports.successResult = successResult diff --git a/src/index.js b/src/index.js index 9e607757..74780305 100644 --- a/src/index.js +++ b/src/index.js @@ -71,6 +71,9 @@ mq.setup((senders) => { // webcompat collection routes let webcompatRoutes = require('./controllers/webcompat').setup(runtime, releases) + // feedback collection routes + let feedbackRoutes = require('./controllers/feedback').setup(runtime, releases) + let server = null // Output request headers to aid in osx crash storage issue @@ -119,7 +122,7 @@ mq.setup((senders) => { server.route( [ common.root - ].concat(releaseRoutes, extensionRoutes, crashes, monitoring, androidRoutes, iosRoutes, braveCoreRoutes, promoProxy, installerEventsCollectionRoutes, webcompatRoutes) + ].concat(releaseRoutes, extensionRoutes, crashes, monitoring, androidRoutes, iosRoutes, braveCoreRoutes, promoProxy, installerEventsCollectionRoutes, webcompatRoutes, feedbackRoutes) ) server.start((err) => { diff --git a/test/feedback.js b/test/feedback.js new file mode 100644 index 00000000..59394ec8 --- /dev/null +++ b/test/feedback.js @@ -0,0 +1,26 @@ +const tap = require('tap') +const moment = require('moment') + +const feedback = require('../src/controllers/feedback') + +tap.test('feedback', (t) => { + let results = feedback.buildStorageObject({ + selection: 'no', + platform: 'androidbrowser', + os_version: 'os', + phone_make: 'make', + phone_model: 'model', + phone_arch: 'arch', + app_version: '1.2.3', + user_feedback: 'feedback' + }) + t.equal(results.platform, 'androidbrowser', 'platform captured') + t.ok(results.ts, 'timestamp inserted') + t.ok(results.ymd, 'ymd inserted') + t.ok(results.id, 'id inserted') + + t.equal(feedback.successResult('1').status, 'ok', 'ok result well formed') + t.ok(feedback.successResult('1').id, 'ok result has id') + + t.done() +}) From a8c9a8389f4668387b8127f46df3fcb7db337db0 Mon Sep 17 00:00:00 2001 From: Aubrey Keus Date: Mon, 6 Apr 2020 12:17:47 -0400 Subject: [PATCH 2/3] Add API key requirement --- package.json | 2 +- src/controllers/feedback.js | 6 +++++- src/verification.js | 15 ++++++++++++++- test/feedback.js | 4 ++++ 4 files changed, 24 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index f3117264..67ee1900 100644 --- a/package.json +++ b/package.json @@ -11,7 +11,7 @@ "start": "./node_modules/.bin/babel-node src/index.js", "lint": "standard", "verify": "node tools/verify.js", - "test": "S3_DOWNLOAD_KEY=1 S3_DOWNLOAD_SECRET=1 BEHIND_FASTLY=1 tap test/*.js", + "test": "S3_DOWNLOAD_KEY=1 S3_DOWNLOAD_SECRET=1 BEHIND_FASTLY=1 API_KEYS=a,b,c tap test/*.js", "test-win": "set BEHIND_FASTLY=1 && tap test/*.js" }, "author": "Brave", diff --git a/src/controllers/feedback.js b/src/controllers/feedback.js index 4b40789a..87743f9f 100644 --- a/src/controllers/feedback.js +++ b/src/controllers/feedback.js @@ -3,6 +3,7 @@ const Boom = require('boom') const moment = require('moment') const storage = require('../storage') const uuid = require('uuid/v4') +const verification = require('../verification') const FEEDBACK_COLLECTION = process.env.FEEDBACK_COLLECTION || 'feedback' @@ -59,7 +60,10 @@ exports.setup = (runtime) => { try { // phase 2 - to be implemented - rate limit on IP address - // phase 2 - to be implemented - callout to referral server to verify api key + // verify API key + if (!verification.isValidAPIKey(request.payload.api_key)) { + return reply(Boom.notAcceptable('invalid api key')) + } // build event const storageObject = buildStorageObject(request.payload) diff --git a/src/verification.js b/src/verification.js index c1d62314..75eb6838 100644 --- a/src/verification.js +++ b/src/verification.js @@ -1,3 +1,4 @@ +const _ = require('underscore') const moment = require('moment') // verification libraries @@ -12,6 +13,13 @@ const verifiers = [ linuxCore.variousVersions, ] +const API_KEYS = _.object( + (process.env.API_KEYS || '') + .split(',') + .map((k) => { return k.trim() }) + .map((k) => { return [k, true] }) +) + // public function to determine is a request should be verified, and if so, // if the usage ping is valid (by iterating over a set of verifiers) const isUsagePingValid = (request, usage, apiKeys = [], tlsSignatures = []) => { @@ -33,7 +41,12 @@ const writeFilteredUsagePing = (mg, usage, cb) => { filteredCollection.insertOne(usage, cb) } +const isValidAPIKey = (k) => { + return !!API_KEYS[k] +} + module.exports = { isUsagePingValid, - writeFilteredUsagePing + writeFilteredUsagePing, + isValidAPIKey, } diff --git a/test/feedback.js b/test/feedback.js index 59394ec8..a530f20c 100644 --- a/test/feedback.js +++ b/test/feedback.js @@ -2,6 +2,7 @@ const tap = require('tap') const moment = require('moment') const feedback = require('../src/controllers/feedback') +const verification = require('../src/verification') tap.test('feedback', (t) => { let results = feedback.buildStorageObject({ @@ -22,5 +23,8 @@ tap.test('feedback', (t) => { t.equal(feedback.successResult('1').status, 'ok', 'ok result well formed') t.ok(feedback.successResult('1').id, 'ok result has id') + t.ok(verification.isValidAPIKey('a'), 'verification key found') + t.notok(verification.isValidAPIKey('z'), 'verification key not found') + t.done() }) From 4883e66a96c8082b05b427168bb5062fd29f1654 Mon Sep 17 00:00:00 2001 From: Kevin Kuehler Date: Mon, 4 May 2020 16:39:29 -0700 Subject: [PATCH 3/3] Minor fixups * assert we use non-deprecated moment.subtract(num, period) * close mongo client and not the connection --- bin/remove-feedback.js | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/bin/remove-feedback.js b/bin/remove-feedback.js index 45fc5e53..d435e28d 100755 --- a/bin/remove-feedback.js +++ b/bin/remove-feedback.js @@ -15,8 +15,10 @@ const args = require('yargs') .argv const main = async (args) => { - MongoClient.connect(mongoURL, (err, connection) => { - const collection = connection.collection(FEEDBACK_COLLECTION); + MongoClient.connect(mongoURL, (err, client) => { + const db = client.db() + + const collection = db.collection(FEEDBACK_COLLECTION); if (args.id) { collection.deleteOne({ id : args.id }, (err, result) => { assert.equal(err, null) @@ -25,11 +27,12 @@ const main = async (args) => { } else { console.log(`Feedback with id ${args.id} removed`) } - connection.close() + client.close() }) } else if (args.interval) { let [num, period] = args.interval.split(' ') num = parseInt(num) + assert(typeof(period) === 'string') const targetYMD = moment().subtract(num, period).format('YYYY-MM-DD') console.log(`This will remove all feedback before ${targetYMD}. Run again with -f to accept`) if (targetYMD && args.f) { @@ -37,13 +40,13 @@ const main = async (args) => { collection.deleteMany({ ymd: { $lt: targetYMD }}, (err, result) => { assert.equal(err, null) console.log(`${result.result.n} feedback records removed`) - connection.close() + client.close() }) } else { - connection.close() + client.close() } } else { - connection.close() + client.close() } }) }