bug: Allow old GCM senderID

Some android clients have an older SenderID. The server needs to allow
and send to that older SenderID since it is extremely difficult to
change this value on the client.

In addtion:
* drops user on bridge update with empty new routing data (there's
nothing to route to, so drop the user)
* fixes comments and var names to be more expicit about what's
happening.
*

closes #549, #579

autopush/endpoint.py

@@ -584,11 +584,23 @@ def _route_notification(self, version, result, data, ttl=None):
     def _router_completed(self, response, uaid_data, warning=""):
         """Called after router has completed successfully"""
         # TODO: Add some custom wake logic here
-
         # Were we told to update the router data?
-        if response.router_data:
+        if response.router_data is not None:
+            if not response.router_data:
+                # An empty router_data object indicates that the record should
+                # be deleted. There is no longer valid route information for
+                # this record.
+                d = deferToThread(self.ap_settings.router.drop_user,
+                                  self.uaid)
+                d.addCallback(lambda x: self._router_response(response))
+                return d
+            # The router data needs to be updated to include any changes
+            # requested by the bridge system.
             uaid_data["router_data"] = response.router_data
+            # set the AWS mandatory data.
             uaid_data["connected_at"] = ms_time()
+            uaid_data["router_type"] = uaid_data.get("router_type",
+                                                     self.router_key)
             d = deferToThread(self.ap_settings.router.register_user,
                               uaid_data)
             response.router_data = None
@@ -597,6 +609,7 @@ def _router_completed(self, response, uaid_data, warning=""):
                                                            warning))
             return d
         else:
+            # No changes are requested by the bridge system, proceed as normal
             if response.status_code == 200 or response.logged_status == 200:
                 self.log.info(format="Successful delivery",
                               **self._client_info)

autopush/router/fcm.py

@@ -45,18 +45,19 @@ def amend_msg(self, msg, data=None):
             msg["senderid"] = data.get('creds', {}).get('senderID')
         return msg
 
-    def register(self, uaid, router_data, router_token=None, *kwargs):
+    def register(self, uaid, router_data, senderid=None, *kwargs):
         """Validate that the FCM Instance Token is in the ``router_data``"""
+        # "token" is the GCM registration id token generated by the client.
         if "token" not in router_data:
             raise self._error("connect info missing FCM Instance 'token'",
                               status=401)
-        # router_token and router_data['token'] are semi-legacy from when
-        # we were considering having multiple senderids for outbound
-        # GCM support. That was abandoned, but it is still useful to
-        # ensure that the client's senderid value matches what we need
-        # it to be. (If the client has an unexpected or invalid SenderID,
+        # senderid is the remote client's senderID value. This value is
+        # very difficult for the client to change, and there was a problem
+        # where some clients had an older, invalid senderID. We need to
+        # be able to match senderID to it's corresponding auth key.
+        # If the client has an unexpected or invalid SenderID,
         # it is impossible for us to reach them.
-        if not (router_token == router_data['token'] == self.senderID):
+        if not (senderid == self.senderID):
             raise self._error("Invalid SenderID", status=410, errno=105)
         # Assign a senderid
         router_data["creds"] = {"senderID": self.senderID, "auth": self.auth}

autopush/router/gcm.py

@@ -12,7 +12,6 @@
 class GCMRouter(object):
     """GCM Router Implementation"""
     log = Logger()
-    gcm = None
     dryRun = 0
     collapseKey = "simplepush"
 
@@ -22,39 +21,45 @@ def __init__(self, ap_settings, router_conf):
         self.min_ttl = router_conf.get("ttl", 60)
         self.dryRun = router_conf.get("dryrun", False)
         self.collapseKey = router_conf.get("collapseKey", "simplepush")
-        self.senderIDs = router_conf.get("senderIDs")
+        self.gcm = {}
+        self.senderIDs = {}
+        # Flatten the SenderID list from human readable and init gcmclient
+        if not router_conf.get("senderIDs"):
+            raise IOError("SenderIDs not configured.")
+        for sid in router_conf.get("senderIDs"):
+            auth = router_conf.get("senderIDs").get(sid).get("auth")
+            self.senderIDs[sid] = auth
+            try:
+                self.gcm[sid] = gcmclient.GCM(auth)
+            except:
+                raise IOError("GCM Bridge not initiated in main")
         self.metrics = ap_settings.metrics
         self._base_tags = []
         self.router_table = ap_settings.router
-        try:
-            sid = self.senderIDs.keys()
-            self.senderID = sid[0]
-            self.auth = self.senderIDs.get(sid[0], {}).get('auth')
-            self.gcm = gcmclient.GCM(self.auth)
-        except:
-            raise IOError("GCM Bridge not initiated in main")
         self.log.debug("Starting GCM router...")
 
     def amend_msg(self, msg, data=None):
         if data is not None:
             msg["senderid"] = data.get('creds', {}).get('senderID')
         return msg
 
-    def register(self, uaid, router_data, router_token=None, *kwargs):
+    def register(self, uaid, router_data, senderid=None, *kwargs):
         """Validate that the GCM Instance Token is in the ``router_data``"""
+        # "token" is the GCM registration id token generated by the client.
         if "token" not in router_data:
             raise self._error("connect info missing GCM Instance 'token'",
                               status=401)
-        # router_token and router_data['token'] are semi-legacy from when
-        # we were considering having multiple senderids for outbound
-        # GCM support. That was abandoned, but it is still useful to
-        # ensure that the client's senderid value matches what we need
-        # it to be. (If the client has an unexpected or invalid SenderID,
+        # senderid is the remote client's senderID value. This value is
+        # very difficult for the client to change, and there was a problem
+        # where some clients had an older, invalid senderID. We need to
+        # be able to match senderID to it's corresponding auth key.
+        # If the client has an unexpected or invalid SenderID,
         # it is impossible for us to reach them.
-        if not (router_token == router_data['token'] == self.senderID):
+        if senderid not in self.senderIDs:
             raise self._error("Invalid SenderID", status=410, errno=105)
         # Assign a senderid
-        router_data["creds"] = {"senderID": self.senderID, "auth": self.auth}
+        router_data["creds"] = {"senderID": senderid,
+                                "auth": self.senderIDs[senderid]}
         return router_data
 
     def route_notification(self, notification, uaid_data):
@@ -99,8 +104,8 @@ def _route(self, notification, uaid_data):
         )
         creds = router_data.get("creds", {"senderID": "missing id"})
         try:
-            self.gcm.api_key = creds["auth"]
-            result = self.gcm.send(payload)
+            gcm = self.gcm[creds['senderID']]
+            result = gcm.send(payload)
         except KeyError:
             raise self._error("Server error, missing bridge credentials " +
                               "for %s" % creds.get("senderID"), 500)

autopush/tests/test_endpoint.py

@@ -618,13 +618,16 @@ def test_put_router_needs_change(self):
         )
         self.sp_router_mock.route_notification.return_value = RouterResponse(
             status_code=500,
-            router_data={},
+            router_data=dict(token="new_connect"),
         )
 
         def handle_finish(result):
             self.assertTrue(result)
             self.endpoint.set_status.assert_called_with(500, None)
-            ok_(not self.router_mock.register_user.called)
+            ru = self.router_mock.register_user
+            ok_(ru.called)
+            eq_('simplepush', ru.call_args[0][0].get('router_type'))
+
         self.finish_deferred.addCallback(handle_finish)
 
         self.endpoint.put(None, dummy_uaid)
@@ -638,13 +641,14 @@ def test_put_router_needs_update(self):
         )
         self.sp_router_mock.route_notification.return_value = RouterResponse(
             status_code=503,
-            router_data=dict(token="new_connect"),
+            router_data=dict(),
         )
 
         def handle_finish(result):
             self.assertTrue(result)
             self.endpoint.set_status.assert_called_with(503, None)
-            assert(self.router_mock.register_user.called)
+            self.router_mock.drop_user.assert_called()
+
         self.finish_deferred.addCallback(handle_finish)
 
         self.endpoint.put(None, dummy_uaid)

autopush/tests/test_router.py

@@ -236,7 +236,7 @@ def setUp(self, fgcm):
     def _check_error_call(self, exc, code):
         ok_(isinstance(exc, RouterException))
         eq_(exc.status_code, code)
-        assert(self.router.gcm.send.called)
+        assert(self.router.gcm['test123'].send.called)
         self.flushLoggedErrors()
 
     def test_init(self):
@@ -250,24 +250,39 @@ def test_init(self):
     def test_register(self):
         result = self.router.register(uaid="uaid",
                                       router_data={"token": "test123"},
-                                      router_token="test123")
+                                      senderid="test123")
         # Check the information that will be recorded for this user
         eq_(result, {"token": "test123",
                      "creds": {"senderID": "test123",
                                "auth": "12345678abcdefg"}})
 
     def test_register_bad(self):
         self.assertRaises(RouterException, self.router.register, "uaid", {})
+        self.assertRaises(RouterException,
+                          self.router.register,
+                          "uaid",
+                          {"token": "abcd1234"},
+                          "test123")
+
+    @patch("gcmclient.GCM")
+    def test_gcmclient_fail(self, fgcm):
+        fgcm.side_effect = Exception
+        settings = AutopushSettings(
+            hostname="localhost",
+            statsd_host=None,
+        )
+        self.assertRaises(IOError, GCMRouter, settings,
+                          {"senderIDs": {"test123": {"auth": "abcd"}}})
 
     def test_route_notification(self):
-        self.router.gcm = self.gcm
+        self.router.gcm['test123'] = self.gcm
         d = self.router.route_notification(self.notif, self.router_data)
 
         def check_results(result):
             ok_(isinstance(result, RouterResponse))
-            assert(self.router.gcm.send.called)
+            assert(self.router.gcm['test123'].send.called)
             # Make sure the data was encoded as base64
-            data = self.router.gcm.send.call_args[0][0].data
+            data = self.router.gcm['test123'].send.call_args[0][0].data
             eq_(data['body'], 'q60d6g')
             eq_(data['enc'], 'test')
             eq_(data['enckey'], 'test')
@@ -276,7 +291,7 @@ def check_results(result):
         return d
 
     def test_ttl_none(self):
-        self.router.gcm = self.gcm
+        self.router.gcm['test123'] = self.gcm
         self.notif = Notification(version=10,
                                   data="q60d6g",
                                   channel_id=dummy_chid,
@@ -286,10 +301,10 @@ def test_ttl_none(self):
 
         def check_results(result):
             ok_(isinstance(result, RouterResponse))
-            assert(self.router.gcm.send.called)
+            assert(self.router.gcm['test123'].send.called)
             # Make sure the data was encoded as base64
-            data = self.router.gcm.send.call_args[0][0].data
-            options = self.router.gcm.send.call_args[0][0].options
+            data = self.router.gcm['test123'].send.call_args[0][0].data
+            options = self.router.gcm['test123'].send.call_args[0][0].options
             eq_(data['body'], 'q60d6g')
             eq_(data['enc'], 'test')
             eq_(data['enckey'], 'test')
@@ -300,7 +315,7 @@ def check_results(result):
         return d
 
     def test_long_data(self):
-        self.router.gcm = self.gcm
+        self.router.gcm['test123'] = self.gcm
         badNotif = Notification(
             10, "\x01abcdefghijklmnopqrstuvwxyz0123456789", dummy_chid,
             self.headers, 200)
@@ -315,22 +330,22 @@ def check_results(result):
         return d
 
     def test_route_crypto_notification(self):
-        self.router.gcm = self.gcm
+        self.router.gcm['test123'] = self.gcm
         del(self.notif.headers['encryption-key'])
         self.notif.headers['crypto-key'] = 'crypto'
         d = self.router.route_notification(self.notif, self.router_data)
 
         def check_results(result):
             ok_(isinstance(result, RouterResponse))
-            assert(self.router.gcm.send.called)
+            assert(self.router.gcm['test123'].send.called)
         d.addCallback(check_results)
         return d
 
     def test_router_notification_gcm_auth_error(self):
         def throw_auth(arg):
             raise gcmclient.GCMAuthenticationError()
         self.gcm.send.side_effect = throw_auth
-        self.router.gcm = self.gcm
+        self.router.gcm['test123'] = self.gcm
         d = self.router.route_notification(self.notif, self.router_data)
 
         def check_results(fail):
@@ -342,7 +357,7 @@ def test_router_notification_gcm_other_error(self):
         def throw_other(arg):
             raise Exception("oh my!")
         self.gcm.send.side_effect = throw_other
-        self.router.gcm = self.gcm
+        self.router.gcm['test123'] = self.gcm
         d = self.router.route_notification(self.notif, self.router_data)
 
         def check_results(fail):
@@ -352,31 +367,31 @@ def check_results(fail):
 
     def test_router_notification_gcm_id_change(self):
         self.mock_result.canonical["old"] = "new"
-        self.router.gcm = self.gcm
+        self.router.gcm['test123'] = self.gcm
         d = self.router.route_notification(self.notif, self.router_data)
 
         def check_results(result):
             ok_(isinstance(result, RouterResponse))
             eq_(result.router_data, dict(token="new"))
-            assert(self.router.gcm.send.called)
+            assert(self.router.gcm['test123'].send.called)
         d.addCallback(check_results)
         return d
 
     def test_router_notification_gcm_not_regged(self):
         self.mock_result.not_registered = {"connect_data": True}
-        self.router.gcm = self.gcm
+        self.router.gcm['test123'] = self.gcm
         d = self.router.route_notification(self.notif, self.router_data)
 
         def check_results(result):
             ok_(isinstance(result, RouterResponse))
             eq_(result.router_data, dict())
-            assert(self.router.gcm.send.called)
+            assert(self.router.gcm['test123'].send.called)
         d.addCallback(check_results)
         return d
 
     def test_router_notification_gcm_failed_items(self):
         self.mock_result.failed = dict(connect_data=True)
-        self.router.gcm = self.gcm
+        self.router.gcm['test123'] = self.gcm
         d = self.router.route_notification(self.notif, self.router_data)
 
         def check_results(fail):
@@ -386,7 +401,7 @@ def check_results(fail):
 
     def test_router_notification_gcm_needs_retry(self):
         self.mock_result.needs_retry.return_value = True
-        self.router.gcm = self.gcm
+        self.router.gcm['test123'] = self.gcm
         d = self.router.route_notification(self.notif, self.router_data)
 
         def check_results(fail):
@@ -406,7 +421,7 @@ def check_results(fail):
     def test_amend(self):
         self.router.register(uaid="uaid",
                              router_data={"token": "test123"},
-                             router_token="test123")
+                             senderid="test123")
         resp = {"key": "value"}
         result = self.router.amend_msg(resp,
                                        self.router_data.get('router_data'))
@@ -416,7 +431,7 @@ def test_amend(self):
     def test_register_invalid_token(self):
         self.assertRaises(RouterException, self.router.register,
                           uaid="uaid", router_data={"token": "invalid"},
-                          router_token="invalid")
+                          senderid="invalid")
 
 
 class FCMRouterTestCase(unittest.TestCase):
@@ -473,7 +488,7 @@ def throw_auth(arg):
     def test_register(self):
         result = self.router.register(uaid="uaid",
                                       router_data={"token": "test123"},
-                                      router_token="test123")
+                                      senderid="test123")
         # Check the information that will be recorded for this user
         eq_(result, {"token": "test123",
                      "creds": {"senderID": "test123",
@@ -629,7 +644,7 @@ def check_results(fail):
     def test_amend(self):
         self.router.register(uaid="uaid",
                              router_data={"token": "test123"},
-                             router_token="test123")
+                             senderid="test123")
         resp = {"key": "value"}
         result = self.router.amend_msg(resp,
                                        self.router_data.get('router_data'))
@@ -639,7 +654,7 @@ def test_amend(self):
     def test_register_invalid_token(self):
         self.assertRaises(RouterException, self.router.register,
                           uaid="uaid", router_data={"token": "invalid"},
-                          router_token="invalid")
+                          senderid="invalid")
 
 
 class SimplePushRouterTestCase(unittest.TestCase):

autopush/tests/test_web_webpush.py

@@ -108,7 +108,8 @@ def test_router_returns_data_without_detail(self):
         def handle_finish(result):
             eq_(result, True)
             self.wp.set_status.assert_called_with(503)
-            assert(self.router_mock.register_user.called)
+            assert(self.router_mock.drop_user.called)
+
         self.finish_deferred.addCallback(handle_finish)
 
         self.wp.post("v1", dummy_token)