bug: label arguments for router.register to prevent misassignment (#648)

Serial argument assignment can lead to misassignment of arguments in
some cases. It's best to be explicit where one can be.

closes #642

autopush/endpoint.py

@@ -703,8 +703,8 @@ def post(self, router_type="", router_token="", uaid="", chid=""):
         self.app_server_key = params.get("key")
         if new_uaid:
             d = Deferred()
-            d.addCallback(router.register, params, router_token,
-                          uri=self.request.uri)
+            d.addCallback(router.register, router_data=params,
+                          reg_id=router_token, uri=self.request.uri)
             d.addCallback(self._save_router_data, router_type)
             d.addCallback(self._create_endpoint)
             d.addCallback(self._return_endpoint, new_uaid, router)
@@ -741,7 +741,8 @@ def put(self, router_type="", router_token="", uaid="", chid=""):
 
         self.add_header("Content-Type", "application/json")
         d = Deferred()
-        d.addCallback(router.register, router_data, uri=self.request.uri)
+        d.addCallback(router.register, reg_id=router_token,
+                      router_data=router_data, uri=self.request.uri)
         d.addCallback(self._save_router_data, router_type)
         d.addCallback(self._success)
         d.addErrback(self._router_fail_err)

autopush/router/fcm.py

@@ -120,21 +120,21 @@ def amend_msg(self, msg, data=None):
             msg["senderid"] = data.get('creds', {}).get('senderID')
         return msg
 
-    def register(self, uaid, router_data, senderid=None, *args, **kwargs):
+    def register(self, uaid, router_data, reg_id=None, *args, **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,
                               uri=kwargs.get('uri'),
-                              senderid=repr(senderid))
+                              senderid=repr(reg_id))
         # 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 (senderid == self.senderID):
+        if not (reg_id == self.senderID):
             raise self._error("Invalid SenderID", status=410, errno=105)
         # Assign a senderid
         router_data["creds"] = {"senderID": self.senderID,

autopush/router/gcm.py

@@ -44,7 +44,7 @@ def amend_msg(self, msg, data=None):
             msg["senderid"] = data.get('creds', {}).get('senderID')
         return msg
 
-    def register(self, uaid, router_data, senderid=None, *args, **kwargs):
+    def register(self, uaid, router_data, reg_id=None, *args, **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:
@@ -56,13 +56,13 @@ def register(self, uaid, router_data, senderid=None, *args, **kwargs):
         # 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 senderid not in self.senderIDs:
+        if reg_id not in self.senderIDs:
             raise self._error("Invalid SenderID", status=410, errno=105,
                               uri=kwargs.get('uri'),
-                              senderid=repr(senderid))
+                              senderid=repr(reg_id))
         # Assign a senderid
-        router_data["creds"] = {"senderID": senderid,
-                                "auth": self.senderIDs[senderid]}
+        router_data["creds"] = {"senderID": reg_id,
+                                "auth": self.senderIDs[reg_id]}
         return router_data
 
     def route_notification(self, notification, uaid_data):

autopush/router/interface.py

@@ -46,7 +46,7 @@ def __init__(self, settings, router_conf):
         the given settings and router conf."""
         raise NotImplementedError("__init__ must be implemented")
 
-    def register(self, uaid, routing_data, *args, **kwargs):
+    def register(self, uaid, router_data, *args, **kwargs):
         """Register the uaid with the connect dict however is preferred and
         return a dict that will be stored as routing_data for this user in the
         future.

autopush/router/simple.py

@@ -48,7 +48,7 @@ def __init__(self, ap_settings, router_conf):
         self.conf = router_conf
         self.waker = None
 
-    def register(self, uaid, connect, *args, **kwargs):
+    def register(self, uaid, *args, **kwargs):
         """Return no additional routing data"""
         return {}
 

autopush/tests/test_endpoint.py

@@ -1879,7 +1879,10 @@ def test_put(self, *args):
         def handle_finish(value):
             self.reg.write.assert_called_with({})
             frouter.register.assert_called_with(
-                dummy_uaid, data, uri=self.reg.request.uri
+                dummy_uaid,
+                reg_id='',
+                router_data=data,
+                uri=self.reg.request.uri
             )
 
         self.finish_deferred.addCallback(handle_finish)

autopush/tests/test_router.py

@@ -98,11 +98,13 @@ def setUp(self):
         self.router_data = dict(router_data=dict(token="connect_data"))
 
     def test_register(self):
-        result = self.router.register("uaid", {"token": "connect_data"})
-        eq_(result, {"token": "connect_data"})
+        result = self.router.register("uaid",
+                                      router_data={"token": "connect_data"})
+        eq_(result, {"token": "connect_data"}),
 
     def test_register_bad(self):
-        self.assertRaises(RouterException, self.router.register, "uaid", {})
+        self.assertRaises(RouterException, self.router.register, "uaid",
+                          router_data={})
 
     def test_route_notification(self):
         d = self.router.route_notification(self.notif, self.router_data)
@@ -249,21 +251,22 @@ def test_init(self):
                           {"senderIDs": {}})
 
     def test_register(self):
-        result = self.router.register(uaid="uaid",
+        result = self.router.register("uaid",
                                       router_data={"token": "test123"},
-                                      senderid="test123")
+                                      reg_id="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,
+        self.assertRaises(RouterException, self.router.register,
                           "uaid",
-                          {"token": "abcd1234"},
-                          "invalid123")
+                          router_data={})
+        self.assertRaises(RouterException,
+                          self.router.register, "uaid",
+                          router_data={"token": "abcd1234"},
+                          reg_id="invalid123")
 
     @patch("gcmclient.GCM")
     def test_gcmclient_fail(self, fgcm):
@@ -447,9 +450,9 @@ def check_results(fail):
         return d
 
     def test_amend(self):
-        self.router.register(uaid="uaid",
+        self.router.register("uaid",
                              router_data={"token": "test123"},
-                             senderid="test123")
+                             reg_id="test123")
         resp = {"key": "value"}
         result = self.router.amend_msg(resp,
                                        self.router_data.get('router_data'))
@@ -458,8 +461,9 @@ def test_amend(self):
 
     def test_register_invalid_token(self):
         self.assertRaises(RouterException, self.router.register,
-                          uaid="uaid", router_data={"token": "invalid"},
-                          senderid="invalid")
+                          "uaid",
+                          router_data={"token": "invalid"},
+                          reg_id="invalid")
 
 
 class FCMRouterTestCase(unittest.TestCase):
@@ -517,16 +521,17 @@ def throw_auth(*args, **kwargs):
         self.assertRaises(IOError, FCMRouter, settings, {})
 
     def test_register(self):
-        result = self.router.register(uaid="uaid",
+        result = self.router.register("uaid",
                                       router_data={"token": "test123"},
-                                      senderid="test123")
+                                      reg_id="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",
+                          router_data={})
 
     def test_route_notification(self):
         self.router.fcm = self.fcm
@@ -695,19 +700,18 @@ def check_results(fail):
         return d
 
     def test_amend(self):
-        self.router.register(uaid="uaid",
-                             router_data={"token": "test123"},
-                             senderid="test123")
+        self.router.register("uaid", router_data={"token": "test123"},
+                             reg_id="test123")
         resp = {"key": "value"}
         result = self.router.amend_msg(resp,
                                        self.router_data.get('router_data'))
         eq_({"key": "value", "senderid": "test123"},
             result)
 
     def test_register_invalid_token(self):
-        self.assertRaises(RouterException, self.router.register,
-                          uaid="uaid", router_data={"token": "invalid"},
-                          senderid="invalid")
+        self.assertRaises(RouterException, self.router.register, "uaid",
+                          router_data={"token": "invalid"},
+                          reg_id="invalid")
 
 
 class SimplePushRouterTestCase(unittest.TestCase):
@@ -745,7 +749,7 @@ def _raise_item_error(self):
         raise ItemNotFound()
 
     def test_register(self):
-        r = self.router.register(None, {})
+        r = self.router.register("uaid", router_data={})
         eq_(r, {})
 
     def test_route_to_connected(self):