From 5f9c32237d764ec33d7ba42e7df62989f90c8c8c Mon Sep 17 00:00:00 2001 From: jrconlin Date: Tue, 9 Aug 2016 13:27:06 -0700 Subject: [PATCH] bug: Update FCM handler to more accurately reflect API FCM, while compatible with GCM, has a much richer error handler. We should also use a fcm specific bridging library. closes #589 --- autopush/router/fcm.py | 177 ++++++++++++++++++++++++---------- autopush/tests/test_router.py | 89 +++++++++-------- doc-requirements.txt | 1 + pypy-requirements.txt | 1 + requirements.txt | 1 + test-requirements.txt | 1 + 6 files changed, 173 insertions(+), 97 deletions(-) diff --git a/autopush/router/fcm.py b/autopush/router/fcm.py index 8cc3150f..a441b43c 100644 --- a/autopush/router/fcm.py +++ b/autopush/router/fcm.py @@ -1,6 +1,5 @@ """FCM Router""" -import gcmclient -import json +import pyfcm from twisted.internet.threads import deferToThread from twisted.logger import Logger @@ -21,6 +20,80 @@ class FCMRouter(object): gcm = None dryRun = 0 collapseKey = "simplepush" + reasonTable = { + "MissingRegistration": { + "msg": ("'to' or 'registration_id' is blank or" + " invalid: {regid}"), + "err": 500, + "errno": 1, + }, + "InvalidRegistration": { + "msg": "registration_id is invalid: {regid}", + "err": 410, + "errno": 105, + }, + "NotRegistered": { + "msg": "device has unregistered with FCM: {regid}", + "err": 410, + "errno": 103, + }, + "InvalidPackageName": { + "msg": "Invalid Package Name specified", + "err": 500, + "errno": 2, + "crit": True, + }, + "MismatchSenderid": { + "msg": "Invalid SenderID used: {senderid}", + "err": 410, + "errno": 105, + "crit": True, + }, + "MessageTooBig": { + "msg": "Message length was too big: {nlen}", + "err": 413, + "errno": 104, + }, + "InvalidDataKey": { + "msg": ("Payload contains an invalid or restricted " + "key value"), + "err": 500, + "errno": 3, + "crit": True, + }, + "InvalidTtl": { + "msg": "Invalid TimeToLive {ttl}", + "err": 400, + "errno": 111, + }, + "Unavailable": { + "msg": "Message has timed out or device is unavailable", + "err": 200, + "errno": 0, + }, + "InternalServerError": { + "msg": "FCM internal server error", + "err": 500, + "errno": 999, + }, + "DeviceMessageRateExceeded": { + "msg": "Too many messages for this device", + "err": 503, + "errno": 4, + }, + "TopicsMessageRateExceeded": { + "msg": "Too many subscribers for this topic", + "err": 503, + "errno": 5, + "crit": True, + }, + "Unreported": { + "msg": "Error has no reported reason.", + "err": 500, + "errno": 999, + "crit": True, + } + } def __init__(self, ap_settings, router_conf): """Create a new FCM router and connect to FCM""" @@ -33,7 +106,7 @@ def __init__(self, ap_settings, router_conf): self.metrics = ap_settings.metrics self._base_tags = [] try: - self.fcm = gcmclient.GCM(self.auth) + self.fcm = pyfcm.FCMNotification(api_key=self.auth) except Exception as e: self.log.error("Could not instantiate FCM {ex}", ex=e) @@ -60,7 +133,8 @@ def register(self, uaid, router_data, senderid=None, *kwargs): 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} + router_data["creds"] = {"senderID": self.senderID, + "auth": self.auth} return router_data def route_notification(self, notification, uaid_data): @@ -72,6 +146,11 @@ def route_notification(self, notification, uaid_data): def _route(self, notification, router_data): """Blocking FCM call to route the notification""" data = {"chid": notification.channel_id} + if not router_data.get("token"): + raise self._error("No registration token found. " + "Rejecting message.", + 410, errno=106, log_exception=False) + regid = router_data.get("token") # Payload data is optional. The endpoint handler validates that the # correct encryption headers are included with the data. if notification.data: @@ -95,29 +174,23 @@ def _route(self, notification, router_data): # registration_ids are the FCM instance tokens (specified during # registration. - router_ttl = notification.ttl or 0 - payload = gcmclient.JSONMessage( - registration_ids=[router_data.get("token")], - collapse_key=self.collapseKey, - time_to_live=max(self.min_ttl, router_ttl), - dry_run=self.dryRun or ("dryrun" in router_data), - data=data, - ) - creds = router_data.get("creds", {"senderID": "missing id"}) + router_ttl = max(self.min_ttl, notification.ttl or 0) try: - self.fcm.api_key = creds["auth"] - result = self.fcm.send(payload) - except KeyError: - raise self._error("Server error, missing bridge credentials " + - "for %s" % creds.get("senderID"), 500) - except gcmclient.GCMAuthenticationError as e: + result = self.fcm.notify_single_device( + collapse_key=self.collapseKey, + data_message=data, + dry_run=self.dryRun or ('dryrun' in router_data), + registration_id=regid, + time_to_live=router_ttl, + ) + except pyfcm.errors.AuthenticationError as e: raise self._error("Authentication Error: %s" % e, 500) except Exception as e: raise self._error("Unhandled exception in FCM Routing: %s" % e, 500) - self.metrics.increment("updates.client.bridge.gcm.attempted", + self.metrics.increment("updates.client.bridge.fcm.attempted", self._base_tags) - return self._process_reply(result) + return self._process_reply(result, notification, router_data) def _error(self, err, status, **kwargs): """Error handler that raises the RouterException""" @@ -125,50 +198,50 @@ def _error(self, err, status, **kwargs): return RouterException(err, status_code=status, response_body=err, **kwargs) - def _process_reply(self, reply): + def _process_reply(self, reply, notification, router_data): """Process FCM send reply""" # acks: # for reg_id, msg_id in reply.success.items(): # updates - for old_id, new_id in reply.canonical.items(): + result = reply.get('results', [])[0] + if reply.get('canonical_ids'): + old_id = router_data['token'] + new_id = result.get('registration_id') self.log.info("FCM id changed : {old} => {new}", old=old_id, new=new_id) - self.metrics.increment("updates.client.bridge.gcm.failed.rereg", + self.metrics.increment("updates.client.bridge.fcm.failed.rereg", self._base_tags) return RouterResponse(status_code=503, response_body="Please try request again.", router_data=dict(token=new_id)) - # naks: - # uninstall: - for reg_id in reply.not_registered: - self.metrics.increment("updates.client.bridge.gcm.failed.unreg", + if reply.get('failure'): + self.metrics.increment("updates.client.bridge.fcm.failed", self._base_tags) - self.log.info("FCM no longer registered: %s" % reg_id) + reason = result.get('error', "Unreported") + err = self.reasonTable.get(reason) + if err.get("crit", False): + self.log.critical( + err['msg'], + nlen=len(notification.data), + regid=router_data["token"], + senderid=self.senderID, + ttl=notification.ttl, + ) + raise RouterException("FCM failure to deliver", + status_code=err['err'], + response_body="Please try request " + "later.") + creds = router_data["creds"] + self.log.info("{msg} : {info}", + msg=err['msg'], + info={"senderid": creds.get('registration_id'), + "reason": reason}) return RouterResponse( - status_code=410, - response_body="Endpoint requires client update", + status_code=err['err'], + errno=err['errno'], + response_body=err['msg'], router_data={}, ) - - # for reg_id, err_code in reply.failed.items(): - if len(reply.failed.items()) > 0: - self.metrics.increment("updates.client.bridge.gcm.failed.failure", - self._base_tags) - self.log.critical("FCM failures: {failed()}", - failed=lambda: json.dumps(reply.failed.items())) - raise RouterException("FCM failure to deliver", status_code=503, - response_body="Please try request later.") - - # retries: - if reply.needs_retry(): - self.log.warn("FCM retry requested: {failed()}", - failed=lambda: json.dumps(reply.failed.items())) - self.metrics.increment("updates.client.bridge.gcm.failed.retry", - self._base_tags) - raise RouterException("FCM failure to deliver, retry", - status_code=503, - response_body="Please try request later.") - - self.metrics.increment("updates.client.bridge.gcm.succeeded", + self.metrics.increment("updates.client.bridge.fcm.succeeded", self._base_tags) return RouterResponse(status_code=200, response_body="Message Sent") diff --git a/autopush/tests/test_router.py b/autopush/tests/test_router.py index ae094ff9..4e34d105 100644 --- a/autopush/tests/test_router.py +++ b/autopush/tests/test_router.py @@ -11,6 +11,7 @@ import apns import gcmclient +import pyfcm from autopush.db import ( Router, @@ -436,7 +437,7 @@ def test_register_invalid_token(self): class FCMRouterTestCase(unittest.TestCase): - @patch("gcmclient.GCM", spec=gcmclient.gcm.GCM) + @patch("pyfcm.FCMNotification", spec=pyfcm.FCMNotification) def setUp(self, ffcm): settings = AutopushSettings( hostname="localhost", @@ -449,7 +450,7 @@ def setUp(self, ffcm): "auth": "12345678abcdefg"} self.fcm = ffcm self.router = FCMRouter(settings, self.fcm_config) - self.headers = {"content-encoding": "aesfcm", + self.headers = {"content-encoding": "aesgcm", "encryption": "test", "encryption-key": "test"} # Payloads are Base64-encoded. @@ -459,30 +460,33 @@ def setUp(self, ffcm): router_data=dict( token="connect_data", creds=dict(senderID="test123", auth="12345678abcdefg"))) - mock_result = Mock(spec=gcmclient.gcm.Result) - mock_result.canonical = dict() - mock_result.failed = dict() - mock_result.not_registered = dict() - mock_result.needs_retry.return_value = False + mock_result = dict( + multicast_id="", + success=0, + failure=0, + canonical_ids=0, + results=[dict()], + ) self.mock_result = mock_result - ffcm.send.return_value = mock_result + ffcm.notify_single_device.return_value = mock_result def _check_error_call(self, exc, code): ok_(isinstance(exc, RouterException)) eq_(exc.status_code, code) - assert(self.router.fcm.send.called) + assert(self.router.fcm.notify_single_device.called) self.flushLoggedErrors() - @patch("gcmclient.GCM", spec=gcmclient.gcm.GCM) - def test_init(self, fgcm): + @patch("pyfcm.FCMNotification", spec=pyfcm.FCMNotification) + def test_init(self, ffcm): settings = AutopushSettings( hostname="localhost", statsd_host=None, ) - def throw_auth(arg): - raise gcmclient.GCMAuthenticationError() - fgcm.side_effect = throw_auth + def throw_auth(*args, **kwargs): + raise Exception("oopsy") + + ffcm.side_effect = throw_auth self.assertRaises(IOError, FCMRouter, settings, {}) def test_register(self): @@ -503,13 +507,14 @@ def test_route_notification(self): def check_results(result): ok_(isinstance(result, RouterResponse)) - assert(self.router.fcm.send.called) + assert(self.router.fcm.notify_single_device.called) # Make sure the data was encoded as base64 - data = self.router.fcm.send.call_args[0][0].data + args = self.router.fcm.notify_single_device.call_args[1] + data = args['data_message'] eq_(data['body'], 'q60d6g') eq_(data['enc'], 'test') eq_(data['enckey'], 'test') - eq_(data['con'], 'aesfcm') + eq_(data['con'], 'aesgcm') d.addCallback(check_results) return d @@ -524,16 +529,16 @@ def test_ttl_none(self): def check_results(result): ok_(isinstance(result, RouterResponse)) - assert(self.router.fcm.send.called) + assert(self.router.fcm.notify_single_device.called) # Make sure the data was encoded as base64 - data = self.router.fcm.send.call_args[0][0].data - options = self.router.fcm.send.call_args[0][0].options + args = self.router.fcm.notify_single_device.call_args[1] + data = args['data_message'] eq_(data['body'], 'q60d6g') eq_(data['enc'], 'test') eq_(data['enckey'], 'test') - eq_(data['con'], 'aesfcm') + eq_(data['con'], 'aesgcm') # use the defined min TTL - eq_(options['time_to_live'], 60) + eq_(args['time_to_live'], 60) d.addCallback(check_results) return d @@ -560,14 +565,14 @@ def test_route_crypto_notification(self): def check_results(result): ok_(isinstance(result, RouterResponse)) - assert(self.router.fcm.send.called) + assert(self.router.fcm.notify_single_device.called) d.addCallback(check_results) return d def test_router_notification_fcm_auth_error(self): - def throw_auth(arg): - raise gcmclient.GCMAuthenticationError() - self.fcm.send.side_effect = throw_auth + def throw_auth(*args, **kwargs): + raise pyfcm.errors.AuthenticationError() + self.fcm.notify_single_device.side_effect = throw_auth self.router.fcm = self.fcm d = self.router.route_notification(self.notif, self.router_data) @@ -577,9 +582,9 @@ def check_results(fail): return d def test_router_notification_fcm_other_error(self): - def throw_other(arg): + def throw_other(*args, **kwargs): raise Exception("oh my!") - self.fcm.send.side_effect = throw_other + self.fcm.notify_single_device.side_effect = throw_other self.router.fcm = self.fcm d = self.router.route_notification(self.notif, self.router_data) @@ -589,41 +594,35 @@ def check_results(fail): return d def test_router_notification_fcm_id_change(self): - self.mock_result.canonical["old"] = "new" + self.mock_result['canonical_ids'] = 1 + self.mock_result['results'][0] = {'registration_id': "new"} self.router.fcm = self.fcm 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.fcm.send.called) + assert(self.router.fcm.notify_single_device.called) d.addCallback(check_results) return d def test_router_notification_fcm_not_regged(self): - self.mock_result.not_registered = {"connect_data": True} + self.mock_result['failure'] = 1 + self.mock_result['results'][0] = {'error': 'NotRegistered'} self.router.fcm = self.fcm 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.fcm.send.called) + assert(self.router.fcm.notify_single_device.called) d.addCallback(check_results) return d def test_router_notification_fcm_failed_items(self): - self.mock_result.failed = dict(connect_data=True) - self.router.fcm = self.fcm - d = self.router.route_notification(self.notif, self.router_data) - - def check_results(fail): - self._check_error_call(fail.value, 503) - d.addBoth(check_results) - return d - - def test_router_notification_fcm_needs_retry(self): - self.mock_result.needs_retry.return_value = True + self.mock_result['failure'] = 1 + self.mock_result['results'][0] = {'error': + 'TopicsMessageRateExceeded'} self.router.fcm = self.fcm d = self.router.route_notification(self.notif, self.router_data) @@ -634,10 +633,10 @@ def check_results(fail): def test_router_notification_fcm_no_auth(self): d = self.router.route_notification(self.notif, - {"router_data": {"token": "abc"}}) + {"router_data": {"token": ""}}) def check_results(fail): - eq_(fail.value.status_code, 500) + eq_(fail.value.status_code, 410) d.addBoth(check_results) return d diff --git a/doc-requirements.txt b/doc-requirements.txt index b60a6270..718c231b 100644 --- a/doc-requirements.txt +++ b/doc-requirements.txt @@ -41,6 +41,7 @@ pyasn1==0.1.9 pyasn1-modules==0.0.8 pycparser==2.14 pycrypto==2.6.1 +pyfcm==1.0.2 pyflakes==1.2.3 python-dateutil==2.5.3 python-jose==0.6.1 diff --git a/pypy-requirements.txt b/pypy-requirements.txt index ee24d705..9c670ca4 100644 --- a/pypy-requirements.txt +++ b/pypy-requirements.txt @@ -40,6 +40,7 @@ pyasn1==0.1.9 pyasn1-modules==0.0.8 pycparser==2.14 pycrypto==2.6.1 +pyfcm==1.0.2 pyflakes==1.2.3 python-dateutil==2.5.3 python-jose==0.6.1 diff --git a/requirements.txt b/requirements.txt index ee24d705..9c670ca4 100644 --- a/requirements.txt +++ b/requirements.txt @@ -40,6 +40,7 @@ pyasn1==0.1.9 pyasn1-modules==0.0.8 pycparser==2.14 pycrypto==2.6.1 +pyfcm==1.0.2 pyflakes==1.2.3 python-dateutil==2.5.3 python-jose==0.6.1 diff --git a/test-requirements.txt b/test-requirements.txt index c5033677..36e51937 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -48,6 +48,7 @@ pyasn1==0.1.9 pyasn1-modules==0.0.8 pycparser==2.14 pycrypto==2.6.1 +pyfcm==1.0.2 pyflakes==1.2.3 python-dateutil==2.5.3 python-jose==0.6.1