Resolved comments

funnel/transports/sms.py

@@ -1,8 +1,6 @@
 """Support functions for sending a short text message."""
 
 
-import uuid
-
 from flask import url_for
 import itsdangerous.exc
 
@@ -23,12 +21,12 @@
 __all__ = ['validate_exotel_token', 'send_via_exotel', 'send_via_twilio', 'send']
 
 
-def make_exotel_token(transactionid: str):
+def make_exotel_token(to: str):
     """Used by :func:`send_via_exotel` to construct a callback URL with a token."""
-    return token_serializer().dumps({'transactionid': transactionid})
+    return token_serializer().dumps({'to': to})
 
 
-def validate_exotel_token(token: str):
+def validate_exotel_token(token: str, to: str):
     """Used by the callback view handler to verify the URL security token."""
     try:
         # Allow 7 days validity for the callback token
@@ -41,10 +39,13 @@ def validate_exotel_token(token: str):
         # Token is invalid
         app.logger.debug("Received invalid Exotel token: %s", token)
         return False
-    if not payload.get('transactionid'):
-        # We expect the token to have a transaction ID, even though it is
-        # fully random. No need to check the validity of it.
-        app.logger.warning("Signed Payload without transaction Id: %s", token)
+
+    phone = payload.get('to')
+    if not phone:
+        app.logger.warning("Signed Payload without Phone number: %s", token)
+        return False
+    if phone != to:
+        app.logger.warning("Signed Payload Phone number does not match : %s", token)
         return False
     return True
 
@@ -62,9 +63,11 @@ def send_via_exotel(phone: str, message: str, callback: bool = True) -> str:
     token = app.config['SMS_EXOTEL_TOKEN']
     payload = {'From': app.config['SMS_EXOTEL_FROM'], 'To': phone, 'Body': message}
     if callback:
-        nonce = make_exotel_token(str(uuid.uuid1()))
         payload['StatusCallback'] = url_for(
-            'process_exotel_event', _external=True, _method='POST', secret_token=nonce
+            'process_exotel_event',
+            _external=True,
+            _method='POST',
+            secret_token=make_exotel_token(phone),
         )
     try:
         r = requests.post(

funnel/views/sms_events.py

@@ -78,7 +78,7 @@ def process_exotel_event(secret_token: str):
     statsd.incr('phone_number.sms.exotel_event.received')
 
     # We just need to verify the token first.
-    if not validate_exotel_token(secret_token):
+    if not validate_exotel_token(secret_token, request.form.get('To')):
         statsd.incr('phone_number.sms.exotel_event.rejected')
         return {'status': 'error', 'error': 'invalid_signature'}, 422
 

tests/unit/transports/test_sms.py

@@ -1,14 +1,10 @@
-import uuid
-
 from flask import Response
 
-from funnel.transports.base import TransportRecipientError
-from funnel.transports.sms import (
-    TransportTransactionError,
-    make_exotel_token,
-    send,
-    validate_exotel_token,
-)
+from mock import patch
+import requests
+
+from funnel.transports.base import TransportConnectionError, TransportRecipientError
+from funnel.transports.sms import make_exotel_token, send, validate_exotel_token
 
 # Target Numbers (Test Only). See this
 # https://www.twilio.com/docs/iam/test-credentials
@@ -65,19 +61,18 @@ def test_exotel_nonce(test_client, test_db_structure):
     """ Test if the exotel nonce works as expected"""
 
     # The random case.
-    nonce = str(uuid.uuid1())
-    token = make_exotel_token(nonce)
-    valid = validate_exotel_token(token)
+    token = make_exotel_token(EXOTEL_TO)
+    valid = validate_exotel_token(token, EXOTEL_TO)
     assert valid
 
     # Pretend that we got another one within X days
-    assert validate_exotel_token(token)
+    assert validate_exotel_token(token, EXOTEL_TO)
 
     # Now call the callback using POST and see if it works.
     # URL and Headers for the post call
     url = '/api/1/sms/exotel_event/' + token
     headers = {'Content-Type': 'application/x-www-form-urlencoded'}
-    data = {'To': '+919999999999', 'SmsSid': 'Some-long-string', 'Status': 'sent'}
+    data = {'To': EXOTEL_TO, 'SmsSid': 'Some-long-string', 'Status': 'sent'}
     with test_client as c:
         resp: Response = c.post(url, headers=headers, data=data)
         assert resp.status_code == 200
@@ -86,10 +81,14 @@ def test_exotel_nonce(test_client, test_db_structure):
         test_db_structure.session.commit()
 
 
-def test_exotel_send(test_client):
+def test_exotel_send_error(test_client):
     """ Only tests if url_for works and usually fails otherwise, which is OK"""
+
+    # Check False Path via monkey patching the requests object
     try:
-        sid = send(EXOTEL_TO, MESSAGE, callback=True)
-        assert sid
-    except TransportTransactionError:
+        with patch.object(requests, 'post') as mock_method:
+            mock_method.side_effect = requests.ConnectionError
+            sid = send(EXOTEL_TO, MESSAGE, callback=True)
+            assert sid
+    except TransportConnectionError:
         assert True