fix(push): do not throw if push fails on the notify endpoint

Fixes #1510
mozilla · Oct 29, 2016 · 90b37d5 · 90b37d5
1 parent 04d9fc5
commit 90b37d5
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 2 deletions.
diff --git a/bin/key_server.js b/bin/key_server.js
@@ -113,7 +113,7 @@ function main() {
                 function (err) {
                   if (err) {
                     log.error({ op: 'server.start.1', msg: 'failed startup with error',
-                                err: { message: err.message } })
+                      err: { message: err.message } })
                     process.exit(1)
                   } else {
                     log.info({ op: 'server.start.1', msg: 'running on ' + server.info.uri })

diff --git a/lib/routes/account.js b/lib/routes/account.js
@@ -1201,12 +1201,25 @@ module.exports = function (
 
         var endpointAction = 'devicesNotify'
         var stringUid = uid.toString('hex')
+
+        function catchPushError(err) {
+          // push may fail due to not found devices or a bad push action
+          // log the error but still respond with a 200.
+          log.error({
+            op: 'Account.devicesNotify',
+            uid: stringUid,
+            error: err
+          })
+        }
+
         return customs.checkAuthenticated(endpointAction, ip, stringUid)
           .then(function () {
             if (body.to === 'all') {
               return push.pushToAllDevices(uid, endpointAction, pushOptions)
+                .catch(catchPushError)
             } else {
               return push.pushToDevices(uid, body.to, endpointAction, pushOptions)
+                .catch(catchPushError)
             }
           })
           .done(

diff --git a/test/local/account_routes.js b/test/local/account_routes.js
@@ -466,7 +466,7 @@ test('/account/device', function (t) {
 })
 
 test('/account/devices/notify', function (t) {
-  t.plan(5)
+  t.plan(6)
   var config = {}
   var uid = uuid.v4('binary')
   var mockRequest = mocks.mockRequest({
@@ -599,6 +599,33 @@ test('/account/devices/notify', function (t) {
       t.equal(err.message, 'Client has sent too many requests')
     })
   })
+
+  t.test('logs error if no devices found', function (t) {
+    t.plan(1)
+    mockRequest.payload = {
+      to: ['bogusid1', 'bogusid2'],
+      TTL: 60,
+      payload: pushPayload
+    }
+
+    var mockLog = mocks.spyLog()
+    var mockPush = mocks.mockPush({
+      pushToDevices: () => P.reject('Devices ids not found in devices')
+    })
+    var mockCustoms = {
+      checkAuthenticated: () => P.resolve()
+    }
+
+    route = getRoute(makeRoutes({
+      customs: mockCustoms,
+      log: mockLog,
+      push: mockPush
+    }), '/account/devices/notify')
+
+    return runTest(route, mockRequest, function (response) {
+      t.equal(JSON.stringify(response), '{}', 'response should not throw push errors')
+    })
+  })
 })
 
 test('/account/device/destroy', function (t) {